Uh oh!
There was an error while loading. Please reload this page.
[opt](recycler) Optimize rowset recycling for recycler - #63295
Conversation
hello-stephen
commented
May 15, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
wyxxxcat
commented
May 19, 2026
run buildall |
hello-stephen
commented
May 19, 2026
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
5f26a57 to
e314138ComparePR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
e314138 to
687a847Comparewyxxxcat
commented
May 28, 2026
run buildall |
wyxxxcat
commented
May 28, 2026
/review |
There was a problem hiding this comment.
I found blocking issues in the rowset recycler changes.
Critical checkpoints:
- Goal/test coverage: The PR aims to optimize rowset recycling and packed-file cleanup. It adds recycler unit coverage, but the PREPARE rowset abort-before-delete ordering and invalid mutable config values are not covered.
- Scope/focus: The core changes are focused on recycler batching, but the new split mark/abort worker changes behavior in a correctness-sensitive path.
- Concurrency: The new mark/abort pool runs independently from delete jobs, creating an ordering race for prepared rowsets.
- Lifecycle/static initialization: No new static initialization lifecycle issue found.
- Configuration: A mutable batch-size config was added without lower-bound handling at use sites.
- Compatibility/persistence: No storage format or RPC compatibility issue found in the reviewed diff.
- Parallel paths: PREPARE rowsets and normal rowsets now diverge; PREPARE loses the old abort-before-delete sequencing.
- Special checks: Per-tablet skipping is present, but the delete batch size needs the same positive clamp pattern used for per-tablet batch size.
- Tests/results: Added tests cover batching behavior, but not the two failure modes above.
- Observability: Existing logs are mostly sufficient; no separate observability blocker found.
- Transaction/persistence/data writes: The PREPARE path can delete physical data before aborting the owning txn/job, which can cause committed metadata to reference missing files.
- Performance: Non-positive delete batch size can force retaining all scanned rowsets in memory until the scan finishes.
User focus: No additional user-provided review focus was specified.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
687a847 to
b5d73d9Comparewyxxxcat
commented
May 28, 2026
run buildall |
wyxxxcat
commented
May 28, 2026
/review |
There was a problem hiding this comment.
I found one additional blocking issue beyond the already-open inline threads.
Critical checkpoint conclusions:
- Goal and tests: the PR aims to throttle/batch recycle rowset work per tablet and avoid deleting packed slice paths directly. The packed-file path looked consistent, but the rowset batching path still misses an important metadata-only case; tests do not cover a large backlog of empty/metadata-only recycle entries.
- Small/focused change: mostly focused, but the recycle_rowsets control flow now splits mark/abort/delete and batching decisions across multiple queues, which introduced ordering issues already noted in existing threads and the additional flush-threshold gap below.
- Concurrency: new worker pools introduce concurrent mark/abort and delete chains. Existing review threads already cover the abort-before-delete race; no additional distinct thread-safety issue was found.
- Lifecycle/static initialization: no new static initialization or non-obvious object lifetime issue found. Worker pools are stopped before final metric reporting.
- Configuration: new mutable batching configs are added. Existing review thread covers lower-bound clamping for recycle_rowsets_delete_batch_size.
- Compatibility: no wire/storage-format incompatibility found.
- Parallel paths: packed file deletion updates both single and batch delete_rowset_data paths. Rowset batching missed the rowset_keys_without_data path, commented inline.
- Conditional checks: the new loop_done early return only checks rowsets.size(), which is insufficient for metadata-only rows.
- Test coverage: tests cover normal rowset batches and small metadata-only counts, but not the large metadata-only backlog that breaks the memory cap.
- Test results: I did not run tests in this review runner.
- Observability: logs were added for scanning and mark/abort jobs; no separate observability blocker found.
- Transaction/persistence/data correctness: existing threads cover the abort-before-delete ordering risk. The new inline comment covers a resource-control/cleanup batching regression.
- User focus: no additional user-provided review focus was specified.
Uh oh!
There was an error while loading. Please reload this page.
wyxxxcat
commented
Jun 3, 2026
run buildall |
hello-stephen
commented
Jun 3, 2026
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
wyxxxcat
commented
Jun 4, 2026
/review |
There was a problem hiding this comment.
I found one additional blocking issue in the added tests. The existing inline threads already cover the recycle_rowsets abort-before-delete ordering and memory/batch-threshold concerns, so I did not duplicate those comments.
Critical checkpoint conclusions:
- Goal/test coverage: The PR aims to limit/batch cloud recycle_rowsets processing and adds targeted tests, but the new test file currently does not compile because it directly accesses a private helper.
- Scope/focus: The production change is focused on recycler batching, though the added unused private declaration should be cleaned up with the test helper fix.
- Concurrency/lifecycle: The modified recycler path introduces additional worker-pool concurrency; existing review threads already cover the main ordering hazards around mark/abort versus deletion.
- Config: Two mutable configs are added; existing review context already covers the missing lower clamp for recycle_rowsets_delete_batch_size.
- Compatibility/storage/transactions: No new persisted format was identified in this pass; rowset deletion ordering concerns remain covered by existing comments.
- Tests/results: Added tests cover batching scenarios, but one added test currently violates C++ access control and will fail compilation.
- Observability/performance: The batching intent is observable through logs/metrics already in the path; no additional distinct issue found beyond the existing batching-memory comments.
User focus: No additional user-provided review focus was specified.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
4.1/4.0 only merge filter packed slice deletion |
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)