branch-4.1: [chore](recycler) Disable enable_mark_delete_rowset_before_recycle and enable_abort_txn_and_job_for_delete_rowset_before_recycle for recycler- #66150 #66150 - #66229
Conversation
…and `enable_abort_txn_and_job_for_delete_rowset_before_recycle` for recycler (apache#66150)
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
I found one blocking data-correctness issue in the recycler default change.
Critical checkpoint conclusions:
- Goal and proof: The diff changes both recycler ownership-safety defaults to false and adapts tests, but every changed lifecycle test forces the previous true/true behavior and does not prove the shipped false/false path safe.
- Scope and clarity: The source change is small, but it removes a correctness-sensitive claim-before-delete protocol without an equivalent fence or a rationale in the PR body.
- Concurrency: Blocking issue found. PREPARE-to-tmp promotion can race with queued object deletion, and an eventual transaction left COMMITTED can lose all tmp rows before lazy finalization and still become VISIBLE.
- Lifecycle/static initialization: The test guard has safe stack lifetime and restores both globals. The relevant production lifecycle is the deferred COMMITTED-to-VISIBLE transition; no separate static-initialization issue was found.
- Configuration: Both flags are mutable.
recycle_tmp_rowsets()observes them in every NORMAL instance mode, while the non-versionedrecycle_rowsets()path also loses the fence. - Compatibility and parallel paths: Versioned ref-count cleanup, retention delays, rolling instance modes, and compaction/schema-change job checks do not provide equivalent ownership. No wire or storage-format incompatibility was found.
- Conditional/error handling: Disabling the mark and abort gates is the blocking condition; no separate error-propagation issue was substantiated.
- Tests/results: The nine guarded lifecycle tests continue to validate true/true only, and existing false/false recycler tests are uncontended. I did not run builds or tests because this review runner explicitly forbids them.
- Observability: Existing logs can trace these paths, but observability does not prevent the race; no separate logging blocker was found.
- Persistence and data writes: Formal or tmp metadata can survive while its object files are deleted, and a transaction can become VISIBLE after its tmp metadata and files are gone.
- Performance: No distinct performance regression was substantiated.
User focus: No additional user-provided review focus was specified.
Review completion: The full two-file authoritative diff and upstream/downstream paths were reviewed. Three rounds of normal and risk-focused review converged; every Round 3 reviewer returned NO_NEW_VALUABLE_FINDINGS against this exact frozen payload, and no suspicious point remains unresolved.
| CONF_mBool(enable_recycle_delete_rowset_key_check, "true"); | ||
| CONF_mBool(enable_mark_delete_rowset_before_recycle, "true"); | ||
| CONF_mBool(enable_abort_txn_and_job_for_delete_rowset_before_recycle, "true"); | ||
| CONF_mBool(enable_mark_delete_rowset_before_recycle, "false"); |
There was a problem hiding this comment.
[P1] Preserve an ownership fence before disabling these defaults
With both defaults false, the non-versioned recycler can select an expired PREPARE rowset and queue object deletion while its recycle key is still present and unmarked. A concurrent commit_rowset() can consume that key and publish tmp metadata, after which the queued worker deletes the now-committed files. The tmp path has the same problem for deferred commits: it can delete/remove all unmarked tmp rows of a transaction left COMMITTED, and TxnLazyCommitTask accepts the later empty scan and advances that transaction to VISIBLE. Versioned ref-counts and retention only narrow the window; they do not establish ownership. Every changed test forces true/true, so none exercises the shipped false/false path. Please keep both fences enabled or add an equivalent atomic claim-before-delete protocol with deterministic race coverage.
pick: #66150