[improve](cloud) Skip non-MOW tablets for get_topn_tablet_delete_bitmap_score#65964
Open
mymeiyi wants to merge 2 commits into
Open
[improve](cloud) Skip non-MOW tablets for get_topn_tablet_delete_bitmap_score#65964mymeiyi wants to merge 2 commits into
mymeiyi wants to merge 2 commits into
Conversation
### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: CloudTabletMgr included non-MOW tablets when calculating the top-N delete bitmap score. Skip non-MOW tablets before reading and aggregating delete bitmap counts so the reported score only represents MOW tables. ### Release note None ### Check List (For Author) - Test: No need to test (one-line predicate change; per request, no test or build was run) - Behavior changed: Yes (delete bitmap score metrics exclude non-MOW tablets) - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines Apache Doris BE Cloud’s CloudTabletMgr::get_topn_tablet_delete_bitmap_score to ensure the reported delete-bitmap “top-N score” reflects only Merge-on-Write (MoW) tablets, by skipping non-MoW tablets before reading/aggregating delete bitmap counts.
Changes:
- Skip tablets that are not enabled for unique-key MoW (
enable_unique_key_merge_on_write()) when computing delete-bitmap counts. - Prevent non-MoW tablets from affecting
total_delete_map_count, max score selection, and the top-N buffer.
Review Checkpoints (Skill Part 1.3)
- Goal & correctness: The change directly enforces the stated goal (exclude non-MoW tablets) by adding an early filter before reading delete bitmap counts.
- Change scope: Minimal and localized (single conditional change inside the traversal handler).
- Concurrency / locks: No new shared-state or lock behavior introduced; the change only adds a short-circuit condition.
- Lifecycle / SIOF: Not applicable (no new statics/globals or cross-TU init changes).
- Config changes: None.
- Compatibility / rolling upgrade: Not applicable (no format/protocol change).
- Parallel code paths: No evidence in this diff of another equivalent scoring path that also needs the same filter.
- Special conditional checks: Condition is straightforward and aligned with the PR intent (MoW-gating before delete-bitmap access).
- Test coverage: No tests added/updated in this PR; the change targets an internal scoring/logging/diagnostic path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gavinchou
previously approved these changes
Jul 23, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
Request changes: the MoW predicate itself is the correct, minimal filter, but the resulting empty-eligible-set lifecycle is not propagated to the exported gauges; see the inline P2 finding.
Critical checkpoint conclusions
- Goal and proof: Non-MoW tablets are removed before total/max aggregation, top-N selection, and the base-rowset phase. The existing metric regression covers only one positive MoW table, so it does not prove mixed membership or the positive-to-zero transition.
- Scope and clarity: The one-line collector change is focused. Completing the behavior requires publishing zero from the cloud callback when a completed scan has no eligible tablets, plus focused coverage.
- Concurrency and lifecycle: The tablet-map snapshot, weak-pointer pinning, immutable mode flag, and delete-bitmap shared lock introduce no new race or lock-order problem. Tablet drop/cache eviction is the concrete lifecycle that exposes the stale-gauge issue.
- Configuration and compatibility: No configuration, protocol, storage-format, persistence, transaction, or FE/BE variable compatibility change is introduced.
- Parallel paths: The shared-nothing implementation is structurally parallel, but its production bitmap persistence/mutation paths are MoW-gated; no concrete second local-path defect was substantiated.
- Performance and observability: Filtering saves bitmap scan/sort/base-rowset work. Observability remains incorrect when zero samples are suppressed, which is the blocking finding.
- Validation: Static review used the authoritative one-file diff and current head
89011a830c6c090edd87f7ce744d27b81dbdfe06. Current formatter, title, license/secret, and macOS BE-UT checks are green. Per the review-runner contract, no local build or test was run. - User focus: No additional user-provided focus was specified; the complete PR and metric lifecycle were reviewed.
### What problem does this PR solve? Issue Number: None Related PR: apache#65964 Problem Summary: When a completed cloud tablet scan returned zero after all eligible MoW tablets were dropped or evicted, the callback skipped updating the delete bitmap score gauges and left stale positive values exposed. Always publish both completed-scan values so the gauges reset to zero when no eligible tablet remains. ### Release note None ### Check List (For Author) - Test: No need to test (straightforward metric assignment change; compilation and tests were not run per request) - Behavior changed: Yes (delete bitmap score gauges now reset to zero when a completed scan has no positive score) - Does this need documentation: No
github-actions
Bot
removed
the
approved
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary
No new blocking findings at head ed02449a5948abb6e06adb76fef56a42346f98b4. The MoW-only filter is correctly placed before the whole-tablet count, maximum, top-N insertion, and base-rowset follow-up. The new unconditional gauge assignments also resolve the stale positive-value defect raised in the existing inline thread by publishing zero after a completed empty/non-MoW-only scan.
Critical checkpoint conclusions
- Goal and proof: Non-MoW CloudTablets can no longer affect the total/max diagnostic values or the top-N base-rowset phase. The existing
metrics_p0/test_delete_bitmap_metrics.groovytest proves only a positive MoW sample; it does not cover mixed membership or the positive-to-zero transition. - Scope and clarity: The two changes are small and focused: one eligibility predicate and the necessary completed-scan gauge publication.
- Concurrency and lifecycle: The periodic scanner snapshots weak tablet references, pins each live tablet before access, reads the immutable production MoW flag, and uses the existing tablet/delete-bitmap locks. Cache eviction, sync, publish, and compaction can make a sample eventual, but do not create unsafe access or a new lock-order issue. Both gauges now follow the latest completed scan, including the drop/eviction-to-zero lifecycle.
- Configuration and compatibility: No configuration, RPC, storage-format, persistence, transaction, or FE/BE variable compatibility change is introduced.
- Parallel paths and conditions: The shared-nothing implementation is structurally parallel but unchanged; no new patch-scoped defect was substantiated there. The early MoW condition is the correct and cheapest filter for this cloud path.
- Performance and observability: Filtering avoids unnecessary bitmap scans, sorting, and base-rowset work for non-MoW tablets. Publishing both values unconditionally restores accurate zero-state observability.
- Tests and validation: No test or expected-result file is changed. The focused positive-to-empty coverage request is already present in the existing line-584 thread, so it is not duplicated. Per the review-runner contract, no local build or test was run. At review time, formatter, CheckStyle, license, secret, and dependency checks were passing; macOS BE UT was still pending.
- Data writes, persistence, and observability support: The patch is diagnostic-only and does not change transactionality, visibility, delete-bitmap persistence, crash recovery, or data-write atomicity. Existing identifiers and logs remain sufficient for this path.
- User focus: No additional user-provided focus was specified; both changed files and their upstream/downstream metric lifecycle were reviewed.
Contributor
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run vault_p0 |
Contributor
TPC-H: Total hot run time: 22866 ms |
Contributor
TPC-DS: Total hot run time: 117159 ms |
Contributor
ClickBench: Total hot run time: 18.56 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem Summary: CloudTabletMgr included non-MOW tablets when calculating the top-N delete bitmap score. Skip non-MOW tablets before reading and aggregating delete bitmap counts so the reported score only represents MOW tables.