Skip to content

[chore](recycler) Disable enable_mark_delete_rowset_before_recycle and enable_abort_txn_and_job_for_delete_rowset_before_recycle for recycler - #66150

Merged
liaoxin01 merged 1 commit into
apache:masterfrom
wyxxxcat:disable_mark
Jul 29, 2026

Conversation

@wyxxxcat

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary:

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@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

@wyxxxcat

Copy link
Copy Markdown
Collaborator Author

run buildall

@wyxxxcat

Copy link
Copy Markdown
Collaborator Author

run buildall

@wyxxxcat

Copy link
Copy Markdown
Collaborator Author

/review

@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 diff changes the two recycler switches to false and adapts tests, but the lifecycle tests force the previous true/true behavior and do not prove the new default is safe.
  • Scope and clarity: The source diff is small, but it changes a correctness-sensitive ownership protocol without an equivalent fence or rationale in the PR description.
  • Concurrency: Blocking issue found. PREPARE-to-tmp promotion can race with queued file deletion, and an eventual COMMITTED transaction can lose its tmp rows before lazy finalization and still become VISIBLE.
  • Lifecycle/static initialization: The relevant non-intuitive lifecycle is eventual commit followed by TxnLazyCommitTask; no separate static-initialization or object-lifetime issue was found.
  • Configuration: Both switches are mutable and observed by recycler scans; making false the production default removes mark-before-delete and abort-before-delete from ordinary non-versioned recycling.
  • Compatibility and parallel paths: No wire/storage-format incompatibility was found. Both non-versioned recycle_rowsets and recycle_tmp_rowsets are affected; versioned recycling follows a separate path.
  • Conditional/error handling: No separate condition or error-propagation bug was found beyond the missing ownership fence.
  • Tests/results: The shortened batching test has internally correct 5 -> 3 -> 1 -> 0 behavior. All nine changed lifecycle tests force true/true, and none deterministically covers the false/false ownership races. I did not run builds or tests because this review runner explicitly forbids them.
  • Observability: Existing logs are sufficient to trace these paths; no separate observability blocker was found.
  • Persistence and data writes: The accepted inline finding can make a transaction VISIBLE after its rowset data has been deleted, violating data correctness. No EditLog path or new cross-process FE/BE variable is involved in this cloud KV change.
  • Performance: No distinct performance regression was substantiated in the reviewed diff.

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

Review completion: The full three-file diff and upstream/downstream paths were reviewed. Three rounds of normal and risk-focused review converged; every Round 3 agent returned NO_NEW_VALUABLE_FINDINGS against the frozen comment set, and no suspicious point remains unresolved.

Comment thread cloud/src/common/config.h

@liaoxin01 liaoxin01 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.

LGTM

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Jul 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@liaoxin01
liaoxin01 merged commit c7e6352 into apache:master Jul 29, 2026
30 checks passed
wyxxxcat added a commit to wyxxxcat/doris that referenced this pull request Jul 29, 2026
…and `enable_abort_txn_and_job_for_delete_rowset_before_recycle` for recycler (apache#66150)
wyxxxcat added a commit to wyxxxcat/doris that referenced this pull request Jul 29, 2026
…and `enable_abort_txn_and_job_for_delete_rowset_before_recycle` for recycler (apache#66150)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants