[fix](cloud) Only mark prepare rowsets before recycle - #65550
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
be1d5e3 to
ebdf1ff
Compare
855b0a4 to
75e6ea1
Compare
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
0bb7bd0 to
0611700
Compare
0611700 to
4e99882
Compare
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: restricting mark-before-delete to PREPARE rowsets is directionally correct, but the new synchronous abort-before-delete path has two job-correctness blockers and one large-backlog performance regression.
Critical checkpoint conclusions:
- Goal and tests: PREPARE is the only production recycle type that can still be promoted by
commit_rowset; one-round COMPACT/DROP cleanup matches their terminal producers. The new unit test and existing job-flow tests cover type selection and basic two-pass behavior, but they do not cover expired jobs, exact selection among parallel/mixed jobs, or scaled abort throughput. Cloud UT passed for the reviewed head. - Scope and compatibility: The patch is focused on three recycler files and adds no wire/storage format or new configuration. Legacy untyped rowsets and tmp-rowset behavior remain compatible.
- Transactions, persistence, and data correctness: The persisted
is_recycledmark conflicts correctly withcommit_rowset, and load-transaction abort failure stops deletion. For job rowsets, however, expired jobs are rejected before ABORT handling, and multi-job records can abort the wrong owner; deletion therefore is not reliably gated on terminating the exact owning job. - Concurrency and lifecycle: The consolidated
SimpleThreadPooldrains queued work before destruction, so no dropped-task or captured-lifetime bug was found. The exact-job selection issue is a real parallel-compaction correctness problem, and running every abort synchronously on the scan thread removes the previous bounded batch concurrency. - Configuration and parallel paths: Existing mutable mark/abort flags were checked in both states. COMPACT, DROP, legacy, and tmp paths are sound; supported parallel compactions and mixed schema-change/compaction records expose the accepted job-selection issue.
- Error handling and observability: Non-OK abort results now prevent physical deletion, which is the right invariant, but deterministic
JOB_EXPIREDbecomes a permanent retry/leak loop. Existing logs identify the rowset and job failure; no new metric or format compatibility concern was found. - Performance: A many-tablet PREPARE backlog can serialize per-row metadata round trips on one scan thread without an aggregate PREPARE cap. Abort work needs a bounded completion barrier before deletion rather than global scan serialization.
No local build or test was run because the review contract prohibits builds; conclusions are based on the authoritative bundled diff, current source tracing, and head CI. No additional user focus was supplied. Because the third and final convergence round caught a payload correction, this review is incomplete under the review contract even though all currently identified candidates have been verified and deduplicated.
4e99882 to
e2be2f0
Compare
|
/review |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 25th, 2026 6:03 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Requesting changes: restricting mark/abort preparation to PREPARE rowsets is correct for the non-versioned path, but the current head still has two additional data-correctness blockers, the already-raised expired-job and serial-abort problems, and a failing mandatory formatter gate.
Critical checkpoint conclusions:
- Goal and tests: The changed non-versioned branch correctly stops marking COMPACT/DROP rowsets, and the new type-selection/batch tests reflect their one-pass cleanup. The goal is incomplete for multi-version instances, which return through a parallel PREPARE path with no mark or owner abort. Tests do not cover that dispatch, the schema-change base/new-tablet identity, or successful expired-job cleanup; the new expired-job test instead codifies the existing failure.
- Scope and compatibility: The patch remains focused on three recycler files and adds no wire/storage format, symbol, or configuration. The exact repeated-compaction selection from the earlier review is fixed on this head. Legacy rowsets, the narrowed tmp-rowset encoding, and former base-version behavior remain compatible.
- Transactions, persistence, and data correctness: In the changed non-versioned path, the persisted
is_recycledwrite andcommit_rowsetuse the same recycle key transactionally, so the mark is an effective promotion barrier. Multi-version PREPARE deletion bypasses that barrier and can race a late commit into metadata that references deleted files. Schema-change output rowsets also rebuild ABORT with the new-tablet index, whichprocess_schema_change_jobrejects before removing either mirrored job record. Expired jobs remain permanently unabortable as already raised in the live thread. - Concurrency and lifecycle:
SimpleThreadPool::stop()drains queued mark/delete work, and no dropped-task, captured-lifetime, lock-order, or deadlock issue was found. Rowset recycling and timeout abort do run concurrently, which makes the multi-version omission actionable. Synchronous per-PREPARE aborts still serialize unbounded metadata round trips on the scan thread, already covered by the live P2 thread. - Configuration and parallel paths: Existing mutable mark/abort flags were checked in both states. COMPACT, DROP, legacy, and non-versioned PREPARE handling are consistent; the enabled multi-version dispatch is the missing functional parallel path. No new FE/BE variable propagation or rolling-upgrade concern applies.
- Error handling and observability: Non-OK owner aborts correctly stop deletion, and current logs carry instance/tablet/job context. The deterministic
JOB_EXPIREDresult becomes a retry/leak loop as already reported. Versioned statistics under-report unmarked PREPARE work, but that is a symptom of the same unsafe bypass rather than a separate root comment. - Performance: Apart from the already-raised loss of bounded abort concurrency, no additional hot-path, allocation, or worker-shutdown regression was substantiated.
- Validation: The review contract prohibits builds and test execution, so none were run locally. A read-only
clang-format-16 --dry-run --Werrorcheck reproduces violations atrecycler.cpp:1930-1931, matching the live failing Clang Formatter job. Current head CI shows BE UT (macOS) passing, but no current Cloud UT result was available.
Review completion: all accepted, dismissed, and duplicate candidates were rechecked against the unchanged live head and the three existing inline threads. The normal full-review and separate risk-focused convergence agents found no further valuable findings for this exact payload. No additional user focus was supplied.
e2be2f0 to
f39b361
Compare
|
/review |
There was a problem hiding this comment.
Requesting changes for head f39b361e4ce8a8d72fe182220490c1855cf6568a. No new inline comments are being added because every remaining blocker is already covered by an existing thread.
Unresolved existing issues:
- P1: apply the PREPARE barrier to versioned recycling. Multi-version instances return through
recycle_versioned_rowsets(), whose PREPARE branch still deletes by prefix without first marking and aborting the owner; a late commit can publish metadata for deleted files. - P1: allow recycler ABORT to remove expired jobs. Both compaction and schema-change processing reject expiration before their ABORT branch, so normally expired PREPARE outputs remain permanently unrecyclable.
- P1: preserve the schema-change base tablet index. A new-tablet output finds the mirrored record but rebuilds the request with the new-tablet index, which cannot drive the base-tablet schema-change ABORT.
- P2: keep PREPARE aborts bounded-concurrent. The scan thread now serializes every PREPARE owner's metadata reads and commit with only a per-tablet cap, so a many-tablet backlog can monopolize a recycle scan.
The earlier exact repeated-compaction selection issue is fixed at this head by copying only the matched compaction, and the formatter issue is resolved by the current successful Clang Formatter check.
Critical checkpoint conclusions:
- Goal and proof: Restricting mark-before-delete to PREPARE is correct for the current-key path. COMPACT and DROP records retire already-formal inputs transactionally and have no live PREPARE owner. The new test proves type selection, but the overall safety goal is not complete while the versioned dispatch and job-abort blockers remain.
- Scope: The patch is confined to the recycler implementation/header and unit tests. The changes are related to the same safety flow, although the unresolved owner-abort corrections are still required for a complete solution.
- Concurrency: Current-key marking conflicts transactionally with
commit_rowset, txn/job abort failures stop deletion, and failed keys retain their data for retry. No lock-order, deadlock, captured-lifetime, or worker-drain issue was found. The remaining concurrency defect is the already-raised unbounded cross-tablet serial abort work. - Lifecycle: The current PREPARE flow is mark on one pass, exact owner abort on a later pass, then physical and KV cleanup.
SimpleThreadPool::stop()drains queued work. The versioned path bypasses that lifecycle barrier, and expired/schema-change owners cannot complete it. - Configuration: No configuration item is added. Existing mutable mark/abort/commit-side checks and force-immediate behavior were traced; the default current-key fence is sound. No PR-introduced configuration regression was found.
- Compatibility: No wire protocol, function symbol, persisted format, or rolling-upgrade contract is changed. Legacy untyped rowsets, base-version handling, and tmp-rowset encoding remain compatible.
- Parallel paths and conditions: Current PREPARE, tmp rowsets, COMPACT, DROP, legacy rowsets, loads, repeated compactions, schema changes, and versioned recycling were checked. The enabled multi-version path is the missing functional parallel path; the
end_version != 1condition is preserved and introduced no distinct issue. - Tests and results: The changed tests cover PREPARE-only marking, one-pass COMPACT batching, and exact later-compaction selection. They do not prove multi-version late-commit exclusion, successful expired-job removal, new-tablet schema-change cleanup, or bounded backlog behavior. The live head shows BE UT (macOS) and Clang Formatter passing, but no current Cloud UT result is present in the check rollup.
- Observability: Existing logs include instance, tablet, version, txn/job, and failure context. The versioned statistics mismatch is a symptom of the same missing barrier rather than a separate finding; no additional metric blocker was substantiated.
- Transactions, persistence, and data writes: Current-key mark/commit conflict handling is atomic, and active exact-compaction removal commits before deletion. Data correctness is still blocked because versioned PREPARE data can be deleted before owner termination, while expired and new-tablet schema-change owners cannot be terminated by this helper.
- FE/BE propagation: No new cross-process variable or protocol field is introduced, so no additional propagation path applies.
- Performance: Apart from the existing bounded-concurrency thread, no new hot-path copy, allocation, or complexity regression was found.
- Other issues: One flag-combination race considered during review was rejected as pre-existing and non-regressive after comparing the aggregate base; this head improves its abort/delete ordering. No other substantiated non-duplicate issue remains.
Validation was static only because the review contract prohibits builds and test execution. The authoritative bundled diff, current source, live head/check state, and existing GitHub review context were inspected. No additional user-provided focus was supplied. The review completed within the three-round limit, and every candidate has been accepted, deduplicated, or dismissed with code evidence.
f39b361 to
b24762b
Compare
3843baf to
69bc479
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for head 69bc479ebca8c54ff92e85700611d5ba65455d40. No new inline comment is being added because the remaining blocker is already covered by an unresolved live thread.
Unresolved blocker:
- P1: apply the PREPARE barrier to versioned recycling. Multi-version instances return from
recycle_rowsets()throughrecycle_versioned_rowsets(), whose PREPARE branch still deletes by prefix without marking the current recycle key, aborting the exact owner, or re-reading the key before deletion. A latecommit_rowset()can therefore promote the same rowset while object deletion is in flight and leave published metadata referencing missing files. The requested multi-version late-commit test is also still absent.
Critical checkpoint conclusions:
- Goal and proof: Restricting mark/abort preparation to PREPARE rowsets is correct on the current-key path. COMPACT and DROP entries are already-terminal formal rowsets. The current head proves PREPARE-only selection, exact txn/job abort, retry behavior, and current-path mark/commit conflict handling, but the overall safety goal is incomplete while the versioned dispatch bypasses that lifecycle.
- Scope: The changes are focused on four Cloud recycler/meta-service files and their unit tests. The additional exact-job, expired-job, schema-mirror, batching, and retry work is part of the same abort-before-delete safety flow.
- Concurrency: On the current-key path, the persisted mark conflicts with
commit_rowset; txn/job abort conflicts with concurrent publish/finish; a fresh recycle-key read suppresses deletion after a winning commit; and failures retain a key for idempotent retry. The sharedSimpleThreadPoolhas bounded workers and queueing and drains onstop(). The remaining actionable concurrency failure is the versioned-path bypass already reported above. - Lifecycle: Current PREPARE cleanup is mark, exact owner abort, fresh-key collection, physical/delete-bitmap cleanup, then recycle-key removal. Exact parallel compactions are selected correctly, expired owners can be aborted only through the internal recycler call, and schema change preserves the base tablet index while clearing both base/new-tablet mirrors atomically. No captured-lifetime, lock-order, deadlock, or partial-retry defect was found.
- Configuration: No new configuration is added. Existing mutable mark/abort flags, base-version compatibility, force-immediate behavior, and multi-version routing were checked. The enabled multi-version route is the missing functional path.
- Compatibility: No RPC field, persisted format, storage format, or cross-version wire contract changes. The internal helper signature defaults expiration checking on, public RPC behavior is unchanged, and legacy untyped/base-version rowsets remain supported.
- Parallel paths and conditions: Current PREPARE, load txn, repeated compaction, schema change, COMPACT, DROP, tmp, legacy, config-disabled, and versioned paths were reviewed. The
end_version != 1condition is preserved. Onlyrecycle_versioned_rowsets()lacks the required PREPARE barrier. - Tests and results: Added tests cover PREPARE-only marking, txn/job abort-before-delete, exact/expired compactions, schema mirrors, compatibility, retry, commit/mark conflict, and 256-key batch boundaries. They do not cover the unresolved multi-version late-commit case. No build or test was run locally because the review contract prohibits it. Live checks for this head show Clang Formatter and BE UT (macOS) passing; no current Cloud UT result is present.
- Observability: Existing logs carry instance, tablet, version, txn/job, key, and failure context. Existing counters remain adequate; no new observability blocker was substantiated.
- Transactions, persistence, and data writes: Current-path mark, owner abort, and job-mirror changes use transactional conflict/commit boundaries and safely retain work after failures. Data correctness is still blocked because versioned PREPARE data can be deleted before its owner is fenced.
- FE/BE propagation: No new FE/BE variable or protocol propagation applies.
- Performance: The earlier serial-abort regression is fixed by bounded worker-pool execution. No additional hot-path, allocation, starvation, or shutdown regression survived review.
- Other issues: A v1 delete-bitmap cleanup concern was verified against the aggregate base and dismissed as unchanged pre-existing behavior; this head improves v2 cleanup retry retention. No distinct new inline issue survived validation and duplicate suppression.
User focus: no additional review focus was supplied, so the complete PR was reviewed without further narrowing.
Review completion: two convergence rounds completed within the three-round cap. All normal and risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS in round 2, every candidate was accepted, deduplicated, or dismissed with code evidence, and the final changed-file/unresolved-candidate sweep found no issue beyond the existing P1 thread.
versioned recycleing will be resolved at another pr |
69bc479 to
261fa33
Compare
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Review completed after two full convergence rounds. One new P1 finding is attached inline.
Critical checkpoint conclusions:
- Goal and scope: The PR is focused on typed PREPARE recycling and exact owner cleanup. The ordinary mark/abort/delete sequence is substantially improved, but mark-only mode still lacks a durable publication fence.
- Architecture and parallel paths: The non-versioned production path and its load, compaction, and schema-change owners were traced end to end. The multi-version bypass is real but already covered by discussion r3622806962, so it is not duplicated here. No static-lifecycle or unrelated FE/BE-variable concern was found.
- Concurrency and lifecycle: The shared worker pool is bounded, and each batch preserves mark -> abort -> reread -> object/delete-bitmap deletion -> recycle-key removal. Same-owner transactional conflicts and failure retries are safe after a successful owner abort. The inline mark-only retry race remains.
- Transactions and data correctness: Exact job selection, expired ABORT handling, schema mirror/base indices, lock cleanup, and final key removal were verified. In the inline interleaving, metadata can still be published after its object files are removed.
- Configuration and compatibility: The mark and abort flags are independently mutable, making
mark=true, abort=falsea supported unsafe combination. No new serialization, storage-format, or rolling-compatibility defect was found. The manufactured[0-1]test case is fileless in real production and was not reported. - Tests and failure paths: I did not run local builds or tests, per the review bundle instructions. The added tests cover ordinary load/job ownership, batching, and retry failures, but do not execute a real late
prepare_rowset()overwrite followed by publication in mark-only mode. - Performance, observability, and documentation: The shared pool restores bounded concurrency; existing logging includes relevant IDs and batch timing. No distinct new performance, observability, or documentation issue was substantiated.
- User focus: No additional review focus was supplied.
Review status: complete and converged, with one new P1 plus the already-existing review threads.
3316c71 to
8cc487f
Compare
207b82e to
94c43c2
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes: the head still introduces one owner-abort concurrency regression, one large-backlog FDB read regression, and two completion-accounting defects. Four new P2 findings are attached inline.
Critical checkpoint conclusions:
- Goal and proof: Restricting deletion to PREPARE rowsets and terminating their exact txn/job owners is directionally correct, and the added tests cover load, compaction, schema change, expiration, and several failure paths. The goal is not fully proved: the same-owner test is unintentionally serialized, and no tests cover the two completion counters or large tmp-rowset reread batches. Existing current-publication and versioned-path blockers remain represented by their live threads.
- Scope and clarity: The implementation is focused on four Cloud meta-service/recycler files and tests, with no unrelated feature expansion. No new config, protocol, storage format, function-symbol contract, or FE/BE variable propagation is introduced.
- Concurrency and lifecycle: Current and tmp cleanup use bounded SimpleThreadPool workers, and stop drains and joins them before captured counters/metrics contexts leave scope. No lock-order, deadlock, static-initialization, or capture-lifetime defect survived review. However, per-key current-PREPARE tasks can concurrently abort the same owner and self-conflict, leaving expired keys for the next normal 3600-second round.
- Configuration, conditions, and parallel paths: Dynamic mark/abort combinations, base-version/no-owner compatibility, current/tmp/versioned collectors, and load/compaction/schema-change owners were traced. The versioned PREPARE bypass is already covered by discussion r3622806962, and the current-key publication fence is already covered by r3755122307; neither is duplicated here.
- Transactions, persistence, and data writes: Exact compaction selection, expired internal ABORT, and schema-change base/mirror cleanup are transactionally conflict-protected. Object and bitmap failures retain a driver key for retry, and worker shutdown is orderly. The tmp-key/ref-count split and two tmp publication races considered during review are real but unchanged from the authoritative baseline, so they are not attributed to this patch. No FE EditLog path applies.
- Error handling and observability: Owner-abort and physical-delete failures retain keys and include instance/tablet/txn/job context. Successful current PREPARE cleanup omits num_recycled, while deferred tmp aborts skip num_expired and metadata-byte accounting, producing internally inconsistent completion reports.
- Tests and results: The added 257-rowset test inherits worker_pool_size=1 from an earlier test and therefore misses production same-owner conflicts; it also never crosses the helper batch because production passes one key per call. No test observes the accepted accounting or FDB-read-amplification paths. I did not run local builds or tests because the review bundle prohibits them; the refreshed live head shows Cloud UT, cloud_p0, compile, formatter, BE/macOS UT, and the other reported checks passing, with only this automated review pending.
- Performance and other issues: The new tmp freshness loop can create and synchronously await up to 10,000 point-read transactions per scan page instead of using the existing bounded-concurrent snapshot batch API. No additional CPU, memory, log-volume, compatibility, documentation, or data-correctness issue survived attribution and duplicate suppression.
User focus: no additional review focus was supplied, so the complete PR was reviewed without further narrowing.
Review completion: capped/incomplete. A new valuable performance finding survived in the third and final allowed round. All currently known candidates are nevertheless verified, deduplicated, accepted, or dismissed, and all four accepted findings are included in this review.
| err = txn->get(key, &val); | ||
| if (err == TxnErrorCode::TXN_KEY_NOT_FOUND) { | ||
| // has already been removed | ||
| int ret = txn_get(txn_kv_.get(), key, val); |
There was a problem hiding this comment.
[P2] Batch the tmp-rowset freshness reads
This loop calls txn_get() once per key; that helper creates a new transaction and synchronously waits for one snapshot point read. A scan page can contain 10,000 expired tmp keys, so one worker can perform 10,000 serial FDB round trips before any object cleanup begins. Keep the post-abort freshness check, but use snapshot batch_get in bounded chunks (the existing API issues up to 1,000 FDB gets concurrently) and retain the per-key missing/parse handling. Please add a large-batch read-count or instrumentation test.
| if (delete_versioned_delete_bitmap_kvs(tablet_id, rowset_id) != 0) { | ||
| return; | ||
| } | ||
| if (txn_remove(txn_kv_.get(), aborted_keys) != 0) { |
There was a problem hiding this comment.
[P2] Count successful abort-protected deletions
After this removal succeeds, the new PREPARE path exits without incrementing num_recycled as the other prefix/formal deletion workers do. An all-PREPARE abort-enabled run can therefore delete every object and KV while its completion log reports num_recycled=0. Update the counter after successful KV removal and cover it in the new PREPARE tests.
| << instance_id_ << " tablet_id=" << rowset_meta->tablet_id() | ||
| << " version=[" << rowset_meta->start_version() << '-' | ||
| << rowset_meta->end_version() << "]"; | ||
| submit_recycle_prepare_rowsets_job(std::string(k), rowset_meta->resource_id(), |
There was a problem hiding this comment.
[P2] Coalesce PREPARE aborts by owner
This submits one abort transaction per rowset, so the default 32 workers race when many PREPARE rowsets share one load txn or tablet job. They all read/write the same owner record; one commit wins and the losers return TXN_CONFLICT, retaining already-expired keys until the next recycler interval (normally 3600s). Group by txn_id or exact (tablet_id, job_id), abort once, and fan the result out to all owned keys. The added 257-key test does not cover this: an earlier test leaks worker_pool_size=1, and each invocation here passes only {key}, so it neither exercises production concurrency nor the 256-item collector batch.
94c43c2 to
bafa160
Compare
bafa160 to
fd77f09
Compare
2 participants
What problem does this PR solve?
Cloud Recycler supports a mark-before-delete flow for rowset recycling. The mark
and abort preparation logic should only apply to PREPARE rowsets, because these
rowsets are deleted by rowset id prefix and may need related txn or job cleanup
before physical data deletion.
Test Coverage
Unit tests cover the following scenarios:
Only PREPARE rowsets are marked before recycling
PREPARErowsets.COMPACTandDROProwsets are not unnecessarily marked.PREPARE rowsets generated by load transactions
PREPARE rowsets generated by compaction jobs
PREPARE rowsets generated by schema change jobs
Expired job handling
Compatibility paths
[0, 1]).Abort failure and retry
Object deletion failure and retry
Commit vs. mark race
Batch boundary
Concurrent tablet recycling
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)