Skip to content

branch-4.0: [chore](recycler) Disable enable_mark_delete_rowset_before_recycle and enable_abort_txn_and_job_for_delete_rowset_before_recycle for recycler (#66150) - #66228

Open
wyxxxcat wants to merge 1 commit into
apache:branch-4.0from
wyxxxcat:pick_66150_4_0

Conversation

@wyxxxcat

Copy link
Copy Markdown
Collaborator

pick: #66150

…and `enable_abort_txn_and_job_for_delete_rowset_before_recycle` for recycler (apache#66150)
@wyxxxcat
wyxxxcat requested a review from morningman as a code owner July 29, 2026 08:20
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@wyxxxcat

Copy link
Copy Markdown
Collaborator Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

Cloud UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 78.60% (1858/2364)
Line Coverage 65.34% (33272/50918)
Region Coverage 65.77% (16436/24989)
Branch Coverage 56.41% (8777/15558)

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one blocking data-correctness issue in the recycler default change.

Critical checkpoint conclusions:

  • Goal and proof: The two-file branch pick flips both recycler ownership switches to false and adapts lifecycle tests, but it does not provide a safe handoff for a durable COMMITTED transaction whose lazy conversion must be recovered.
  • Scope and clarity: Both changed files and all related recycler, transaction, lazy commit, compaction, and schema-change paths were reviewed. The branch-pick goal is clear; the PR/source descriptions do not explain why removing the safety protocol is correct.
  • Concurrency: Blocking issue M-001 is accepted. Tmp recycling and COMMITTED advancement run concurrently, and no KV transaction spans object deletion, tmp-key removal, and the visibility transition.
  • Lifecycle/static initialization: An interrupted lazy commit can remain COMMITTED until recycler eligibility, then become VISIBLE after its tmp rows are deleted. The new test guard's construction/destruction is sound; no separate static-lifetime issue was found.
  • Configuration: The mutable false/false defaults affect the common tmp-rowset path in both versioned and non-versioned modes. Preserving marking is a minimum barrier, but deletion also needs a status-aware COMMITTED-owner check.
  • Compatibility and parallel paths: Old-BE/delete-predicate, missing-job-id, direct transaction, compaction, schema-change, versioned formal-rowset, and parallel lazy commit paths were checked. Normal expiry gates qualify the direct publisher races; no separate compatibility blocker was found.
  • Conditional/error handling: Marked tmp rows stop lazy commit, but the recycler abort helper ignores COMMITTED owners, while an empty lazy scan is treated as success and followed by the VISIBLE transition.
  • Tests/results: All nine modified lifecycle tests force the previous true/true behavior, so none covers false/false recovery of a stale COMMITTED transaction. I did not run builds or tests because this review runner explicitly forbids them.
  • Observability: Existing logs can trace recycler and lazy-commit steps but do not prevent the invalid state transition.
  • Persistence and data writes: M-001 can persist VISIBLE transaction metadata without rowset files and can leave a partition's pending transaction id uncleared.
  • Performance: No distinct performance regression was substantiated.
  • Repository instructions: No required AGENTS.md files were listed, and the repository-local code-review skill was absent; the prompt contract was followed without reducing review coverage.

User focus: No additional user-provided focus was specified.

Review completion: The full authoritative two-file diff and relevant upstream/downstream paths were reviewed. Three normal/risk-focused rounds converged; every Round 3 reviewer returned NO_NEW_VALUABLE_FINDINGS against this frozen one-inline comment set, and no suspicious point remains unresolved.

Comment thread cloud/src/common/config.h
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");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Keep COMMITTED tmp rowsets fenced from recycling

With both defaults false, recycle_tmp_rowsets() can delete and remove every tmp row of a transaction whose status is already COMMITTED but whose lazy conversion was interrupted (for example across restart). do_recycle() runs tmp recycling and abort_timeout_txn() concurrently; if recycling wins, TxnLazyCommitTask::commit() gets an empty scan, performs no partition conversion, and still calls make_committed_txn_visible(). The transaction becomes VISIBLE while its rowset files and metadata are gone, and its pending partition id can remain uncleared. No KV conflict spans object deletion, tmp-key removal, and that visibility transition; the ref-count key is removed only after data deletion. All nine modified lifecycle tests force true/true, so none exercises this default. Please at least preserve mark-before-delete and make deletion of COMMITTED tmp rows status-aware (advance or recheck the owner before physical deletion), with deterministic false/false recovery coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants