Skip to content

[fix](checker) Avoid false-positive leaked delete bitmaps for unexpired job tmp rowsets - #64313

Merged
hello-stephen merged 2 commits into
apache:masterfrom
mymeiyi:fix-dbm-checker
Jun 30, 2026
Merged

[fix](checker) Avoid false-positive leaked delete bitmaps for unexpired job tmp rowsets#64313
hello-stephen merged 2 commits into
apache:masterfrom
mymeiyi:fix-dbm-checker

Conversation

@mymeiyi

Copy link
Copy Markdown
Contributor

The delete bitmap inverted check reported a delete bitmap as leaked when its owning rowset was not found among the committed rowsets / pending delete bitmaps of the tablet. However, a compaction job writes delete bitmaps keyed by its tmp output rowset id before the job commits and makes the rowset visible. During that window the delete bitmap exists while the rowset is not yet in the committed list, so it was wrongly flagged as leaked.

Add InstanceChecker::collect_unexpired_job_tmp_rowsets() to gather, per tablet, the rowset ids of job tmp rowsets that are still alive from the recycler's point of view, and skip them during the inverted check. The same calculate_tmp_rowset_expired_time() threshold as the recycler is reused (now exposed via recycler.h) so a delete bitmap is never reported as leaked while its tmp rowset is still alive from the recycler's view.

…red job tmp rowsets
The delete bitmap inverted check reported a delete bitmap as leaked when its
owning rowset was not found among the committed rowsets / pending delete
bitmaps of the tablet. However, a compaction/SC job writes delete bitmaps
keyed by its tmp output rowset id before the job commits and makes the rowset
visible. During that window the delete bitmap exists while the rowset is not
yet in the committed list, so it was wrongly flagged as leaked.
Add InstanceChecker::collect_unexpired_job_tmp_rowsets() to gather, per tablet,
the rowset ids of job tmp rowsets that are still alive from the recycler's
point of view, and skip them during the inverted check. The same
calculate_tmp_rowset_expired_time() threshold as the recycler is reused (now
exposed via recycler.h) so a delete bitmap is never reported as leaked while
its tmp rowset is still alive from the recycler's view.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings June 9, 2026 10:28
@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?

CopilotAI reviewed Jun 9, 2026

CopilotAI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

CopilotAI 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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment threadcloud/src/recycler/recycler.h
Comment threadcloud/src/recycler/checker.cpp
@mymeiyi

Copy link
Copy Markdown
ContributorAuthor

/review

github-actions[bot]
github-actionsBot previously requested changes Jun 9, 2026

@github-actionsgithub-actionsBot 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.

Review conclusion: request changes.

Critical checkpoints:

  • Goal/test coverage: The PR aims to avoid false leaked-delete-bitmap reports for still-live job tmp rowsets and adds focused unit coverage for unexpired, expired, and non-job tmp rowsets. The main path is covered, but the new overflow path is not safely handled.
  • Scope/focus: The change is small and localized to the Cloud recycler checker.
  • Concurrency/lifecycle: No new shared mutable state or long-lived lifecycle issue was found; this is a synchronous checker scan.
  • Configuration/compatibility: No new config or storage/protocol compatibility change.
  • Parallel paths: The checker now reuses the recycler tmp-rowset expiration helper, which is appropriate.
  • Error handling/data correctness: One issue: when the collection cap is reached, the checker continues with incomplete tmp-rowset state and can report live delete bitmaps as leaked.
  • Testing: Positive/negative tests were added, but the cap/overflow case that causes false positives is missing.
  • Observability/performance: Logging exists for the cap, but logging does not prevent an incorrect checker result.

User focus: No additional user-provided focus points were listed.

Comment threadcloud/src/recycler/checker.cpp
@github-actionsgithub-actionsBot added the approved Indicates a PR has been approved by one committer. label Jun 9, 2026
@github-actions

Copy link
Copy Markdown
Contributor

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

@mymeiyi

Copy link
Copy Markdown
ContributorAuthor

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

Cloud UT Coverage Report

Increment line coverage 71.82% (79/110) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage78.40% (1916/2444)
Line Coverage64.89% (34282/52828)
Region Coverage65.32% (17636/26998)
Branch Coverage54.00% (9372/17354)

@hello-stephen
hello-stephen merged commit 8bcaabb into apache:masterJun 30, 2026
34 checks passed
github-actionsBot pushed a commit that referenced this pull request Jun 30, 2026
…ed job tmp rowsets (#64313)
The delete bitmap inverted check reported a delete bitmap as leaked when
its owning rowset was not found among the committed rowsets / pending
delete bitmaps of the tablet. However, a compaction job writes delete
bitmaps keyed by its tmp output rowset id before the job commits and
makes the rowset visible. During that window the delete bitmap exists
while the rowset is not yet in the committed list, so it was wrongly
flagged as leaked.
Add InstanceChecker::collect_unexpired_job_tmp_rowsets() to gather, per
tablet, the rowset ids of job tmp rowsets that are still alive from the
recycler's point of view, and skip them during the inverted check. The
same calculate_tmp_rowset_expired_time() threshold as the recycler is
reused (now exposed via recycler.h) so a delete bitmap is never reported
as leaked while its tmp rowset is still alive from the recycler's view.
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
yiguolei pushed a commit that referenced this pull request Jun 30, 2026
… for unexpired job tmp rowsets #64313 (#65012)
Cherry-picked from #64313
Co-authored-by: meiyi <meiyi@selectdb.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@yiguoleiyiguolei mentioned this pull request Jul 7, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approvedIndicates a PR has been approved by one committer.dev/4.1.3-merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@mymeiyi@hello-stephen@gavinchou@luwei16@yiguolei