wyxxxcat commented on PR #65550: URL: https://github.com/apache/doris/pull/65550#issuecomment-5237120389
> 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](https://github.com/apache/doris/pull/65550#discussion_r3622806962). Multi-version instances return from `recycle_rowsets()` through `recycle_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 late `commit_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 shared `SimpleThreadPool` has bounded workers and queueing and drains on `stop()`. 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 != 1` condition is preserved. Only `recycle_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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
