[improvement](cloud) Remove pre-rowset delete bitmaps by key - #66598
Open
mymeiyi wants to merge 1 commit into
Open
[improvement](cloud) Remove pre-rowset delete bitmaps by key#66598mymeiyi wants to merge 1 commit into
mymeiyi wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Cloud cumulative compaction’s cleanup of pre-rowset delete bitmaps by switching from broad range clears to an (optionally) key-based removal path that reports per-(rowset, segment, version) bitmap sizes to Meta Service, enabling bounded, transaction-size-aware deletes while maintaining backward compatibility.
Changes:
- Extend
UpdateDeleteBitmapRequestwith per-rowset delete-bitmap statistics (PreRowsetDeleteBitmapStatsPB) to drive key-based cleanup. - Add
blob_remove_keys()and reuse a sharedDEFAULT_BLOB_SPLIT_SIZEto deterministically reconstruct legacy + split blob keys for removal. - Implement Meta Service key-based pre-rowset delete-bitmap removal (with batching) and add tests covering batching, underestimated sizes, and request encoding.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| gensrc/proto/cloud.proto | Adds PreRowsetDeleteBitmapStatsPB and wires it into UpdateDeleteBitmapRequest. |
| cloud/test/meta_service_test.cpp | Adds tests for key-based removal behavior, batching, and underestimated-size tail handling. |
| cloud/test/blob_message_test.cpp | Adds test ensuring legacy (unsuffixed) blob values can be removed via reconstructed key lists. |
| cloud/src/meta-store/blob_message.h | Introduces DEFAULT_BLOB_SPLIT_SIZE and declares blob_remove_keys(). |
| cloud/src/meta-store/blob_message.cpp | Implements blob_remove_keys() and refactors split key construction via blob_key(). |
| cloud/src/meta-service/meta_service.cpp | Adds key-based pre-rowset delete-bitmap removal and stricter delete-bitmap key decoding in reads. |
| be/test/cloud/cloud_tablet_test.cpp | Adds unit test ensuring compaction aggregation returns expected pre-rowset stats. |
| be/test/cloud/cloud_meta_mgr_test.cpp | Adds unit test verifying request encoding for pre-rowset delete-bitmap stats. |
| be/src/common/config.h | Declares enable_remove_pre_rowsets_delete_bitmap_by_keys. |
| be/src/common/config.cpp | Defines the new config flag (default enabled) and documents intent. |
| be/src/cloud/cloud_tablet.h | Adds PreRowsetDeleteBitmapStats type and extends aggregation API signature. |
| be/src/cloud/cloud_tablet.cpp | Collects serialized-size stats for original bitmaps within the aggregation version range. |
| be/src/cloud/cloud_meta_mgr.h | Extends MetaMgr RPC helper signature to accept pre-rowset stats. |
| be/src/cloud/cloud_meta_mgr.cpp | Encodes pre-rowset stats into UpdateDeleteBitmapRequest when provided. |
| be/src/cloud/cloud_cumulative_compaction.cpp | Enables collection/sending of pre-rowset stats behind the new config flag. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![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
Requesting changes for two P1 reader-safety regressions in the new point-delete cleanup path:
- A new Meta-Service can persist a nonzero-first split tail that only the new reader skips. During a rolling deployment, an old MS sharing the same FDB can return that fragment as a complete bitmap; the default-enabled BE switch has no fleet capability or rollout fence.
- After
TXN_TOO_OLDreplaces a paginated read transaction, the handler keeps its partially assembled bitmap state. A point-cleanup commit between snapshots can remove unseen suffixes while leaving later ones, and the same-group branch appends across that sequence gap.
Checkpoint conclusions:
- Goal and scope: Reviewed the full 15-file change and the BE producer, Meta-Service writer/reader, blob-key, proto, retry, and test paths. Point-delete batching addresses the FDB transaction-size goal, and aggregate-before-delete ordering is sound, but the two reader-safety blockers remain.
- Concurrency and transactions: Atomic commits and deterministic retries keep replacement aggregates durable across partial cleanup. The accepted homogeneous-version failure is instead the fresh-snapshot pagination interleaving described inline; no additional lock-order or producer-snapshot defect survived review.
- Lifecycle, configuration, and compatibility: Ownership and request lifetimes are scoped correctly, with no new static-initialization or reference-cycle issue. The mutable BE kill switch preserves legacy range cleanup when disabled, and ordinary old-BE/new-MS plus new-BE/old-MS request handling is additive; mixed old/new MS readers are not safe because there is no fleet gate.
- Data and parallel paths: Store versions 1 and 3 use the affected v1 path; version 2 bypasses it. Recycler/checker and other adjacent consumers did not expose a third deserialization path. The missing proto2 nested-field validation issue is already covered by the existing inline thread and was suppressed as a duplicate.
- Tests: Added tests cover request encoding,
[start,end)statistics, legacy/empty behavior, exact batching, and underestimated retained tails. Missing coverage is a pre-change reader over retained state and a pagination +TXN_TOO_OLD+ concurrent cleanup interleaving. I did not run builds or tests because the supplied review bundle explicitly prohibits them. - Performance and observability: The
P*S*Vstats materialization remains a worthwhile stress concern, but no distinct correctness or severe bounded regression was substantiated. Existing logs identify cleanup failures and skipped tails.
The review converged in three rounds: all three final-round reviewers returned NO_NEW_VALUABLE_FINDINGS, and no issue beyond the two inline comments survived validation and live duplicate suppression.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
mymeiyi
added a commit
to mymeiyi/doris
that referenced
this pull request
Aug 10, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66598 Problem Summary: Cloud cumulative compaction previously removed aggregated pre-rowset delete bitmaps with a Meta Service range clear. This could create a large FoundationDB transaction and remove keys outside the exact reported bitmap set. Collect each original bitmap by segment within the aggregation version range and let Meta Service remove the reported legacy and split blob keys individually in bounded transactions. Requests without statistics retain range deletion for compatibility. A paginated delete bitmap read could also retain fragments from an expired transaction and append fragments from a replacement transaction after concurrent point cleanup, returning a gapped or truncated bitmap. Track the first blob key for the active bitmap, verify it in the replacement transaction, discard obsolete partial entries when it no longer exists, and validate blob sequence continuity. Suppress removal logs when no key is deleted. ### Release note Enable bounded, key-based removal of pre-rowset delete bitmap blobs by default during Cloud cumulative compaction. Prevent incomplete delete bitmaps from being returned when an FDB read transaction is replaced during concurrent point cleanup. Set enable_remove_pre_rowsets_delete_bitmap_by_keys=false to retain range deletion. ### Check List (For Author) - Test: Not run (per request); added BE and Meta Service unit coverage, including retained-tail and complete-removal snapshot retry interleavings - Behavior changed: Yes. Pre-rowset delete bitmap removal uses reported keys and bounded transactions by default; incomplete obsolete bitmap fragments are discarded after transaction replacement; empty removal logs are suppressed. - Does this need documentation: No
mymeiyi
force-pushed
the
delete-dmb-key
branch
from
August 10, 2026 08:32
ca920c5 to
c2b0e80
Compare
Contributor
Author
|
/review |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
There was a problem hiding this comment.
Review summary
Requesting changes for two distinct issues on the current head:
- The Meta-Service point-cleanup endpoint does not enforce that every supplied stat version is in the source interval. A fully populated request can therefore clear the replacement at
end_versionafter writing it. - V2-only delete-bitmap storage still performs the new full source-stat scan and materialization even though the callee discards it before issuing any v1 request.
Critical-checkpoint conclusions
- Goal and proof: The change generally achieves transaction-batched point cleanup for valid v1/dual-write compaction requests. The destructive RPC range invariant and the v2-only no-op cost remain. Added unit tests cover stat collection/encoding, legacy and split blob keys, transaction batching, underestimated tails, and pagination interleavings, but not an end-version stat or the v2-only skip.
- Scope and focus: The 15-file change is focused on the BE producer, protobuf contract, Meta-Service writer/reader, blob-key helper, configuration, and their tests. There was no additional user-provided review focus.
- Concurrency and locks: BE bitmap enumeration and aggregation use
DeleteBitmap's shared lock; no new lock-order or deadlock issue survived review. FDB readers can overlap cleanup and replace transactions. The residual same-key, fresh-snapshot assembly concern has the same root cause and remedy as the existingdiscussion_r3746653971thread, so it was not duplicated. The v2-only scan unnecessarily lengthens shared-lock work. - Lifecycle and initialization: Request-owned stats and transaction objects have bounded lifetimes, every transaction replacement is checked, and no new reference cycle, ownership leak, or cross-TU static-initialization dependency was found.
- Configuration: The new kill switch is mutable and sampled per compaction, with the legacy range-removal fallback when stats are absent. Store mode 2 is independently supported, but still pays the unused stats cost described inline.
- Compatibility: Protobuf field 19 is additive. Old BE to new MS falls back to range cleanup, new BE to old MS has the unknown field ignored, and modes 1/3 retain a v1 consumer. The author-stated all-MS-before-BE rollout fence addresses the old-reader retained-tail discussion; no duplicate compatibility comment was added.
- Parallel and conditional paths: Cumulative compaction is the only producer of these stats; base/full compaction do not need the post-commit v1 helper. Valid BE construction uses
[start,end), rowset-nonexistence and empty-stats cases were traced, and the missing server-side enforcement of that same interval is the data-correctness issue inline. - Error handling and memory safety: Changed
TxnErrorCoderesults are checked and include useful tablet/rowset context. No uncheckedStatus, exception-boundary, ownership, nullable-column, or valid-input allocation defect survived review. - Tests and results: The new unit tests exercise the principal positive and retry paths, but the two inline cases need negative/config-specific coverage. No regression result file is applicable. I did not run builds or tests because the supplied review bundle explicitly prohibits them.
- Observability: Per-batch logs, transaction statistics, failure counters, and identifiers are sufficient for the new cleanup path; no additional metric blocker was identified.
- Transactions, persistence, and data writes: Aggregate writes precede every source clear, the first cleanup commit carries any buffered aggregate, later commits are clear-only, and whole-handler retries are same-key idempotent. There is no FE EditLog path. The unchecked end-version stat breaks the logical aggregate-before-delete guarantee despite a successful atomic commit.
- Cross-process values: The new BE-to-MS protobuf data is populated and consumed on all v1-writing paths with an absent-field fallback; FE is not involved. The endpoint must still validate the transmitted cross-field range before destructive reconstruction.
- Performance and remaining issues: The distinct performance regression is the O(K) v2-only stats scan/allocation. A final sweep found no other substantiated issue beyond the two inline comments and the three already-existing validation/reader threads.
No builds or tests were run, as required by the review bundle.
mymeiyi
added a commit
to mymeiyi/doris
that referenced
this pull request
Aug 10, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66598 Problem Summary: Cloud cumulative compaction previously removed aggregated pre-rowset delete bitmaps with a Meta Service range clear. This could create a large FoundationDB transaction and remove keys outside the exact reported bitmap set. Collect each original bitmap by segment within the aggregation version range and let Meta Service remove the reported legacy and split blob keys individually in bounded transactions. Requests without statistics retain range deletion for compatibility. A paginated delete bitmap read could also retain fragments from an expired transaction and append fragments from a replacement transaction after concurrent point cleanup, returning a gapped or truncated bitmap. Track the first blob key for the active bitmap, verify it in the replacement transaction, discard obsolete partial entries when it no longer exists, and validate blob sequence continuity. Suppress removal logs when no key is deleted. V2-only cumulative compaction skips the unused V1 pre-rowset aggregation before collecting source statistics. Meta Service also asserts that BE-produced cleanup statistic versions remain inside the compaction source interval. ### Release note Enable bounded, key-based removal of pre-rowset delete bitmap blobs by default during Cloud cumulative compaction. Prevent incomplete delete bitmaps from being returned when an FDB read transaction is replaced during concurrent point cleanup. V2-only writes skip unused V1 pre-rowset aggregation. Set enable_remove_pre_rowsets_delete_bitmap_by_keys=false to retain range deletion. ### Check List (For Author) - Test: Not run (per request); added BE and Meta Service unit coverage, including retained-tail and complete-removal snapshot retry interleavings - Behavior changed: Yes. Pre-rowset delete bitmap removal uses reported keys and bounded transactions by default; incomplete obsolete bitmap fragments are discarded after transaction replacement; V2-only writes skip unused V1 aggregation; empty removal logs are suppressed. - Does this need documentation: No
mymeiyi
force-pushed
the
delete-dmb-key
branch
from
August 10, 2026 09:54
c2b0e80 to
b321913
Compare
Contributor
TPC-H: Total hot run time: 33647 ms |
Contributor
Author
|
/review |
Contributor
ClickBench: Total hot run time: 23.9 s |
Contributor
There was a problem hiding this comment.
Review status: complete after two full rounds; changes requested.
The point-delete design is focused and the valid-request transaction/read state machine now looks consistent, but two new blocking RPC-boundary issues remain in addition to the already-open validation threads.
Critical checkpoints:
- Goal and tests: BE collection, protobuf encoding, point batching, tail handling, and
TXN_TOO_OLDbehavior are positively covered, but hostile negative cases for replacement ownership and bounded expansion are missing. - Data correctness and transactions: for producer-valid requests, aggregate puts commit no later than source deletes and retries are idempotent. However, cleanup stats are not bound to a verified end-version replacement, so a fully populated request can delete live marks with no preserved aggregate.
- Error handling and resource bounds: claimed bitmap sizes are expanded into key vectors without storable-size or cumulative-work bounds, permitting Meta-Service memory or CPU exhaustion before batching.
- Concurrency and lifecycle: no new lock-order, race, static-initialization, or ownership issue was found.
- Configuration and compatibility: v2 work is gated; v1 and v3 use the path; field 19 is additive. The old-reader retained-tail concern remains governed by the stated all-MS-before-BE rollout and its existing thread.
- Parallel paths and performance: cumulative-only placement is appropriate; base and full compaction have no equivalent retained pre-rowsets. The intended v1/v3 O(K) scan is necessary for point cleanup.
- Observability: existing contextual logs and RPC metrics are sufficient.
- Review execution: static review only, as required by the review runner; no builds or tests were run.
User focus: no additional user-provided focus was supplied, so the full PR was reviewed.
| return true; | ||
| } | ||
|
|
||
| for (const auto& rowset_stats : request->pre_rowset_delete_bitmap_stats()) { |
Contributor
There was a problem hiding this comment.
[P1] Bind every cleanup stat to its replacement
The stats list is independent of the aggregate-output arrays, so a fully populated request can name (rowset B, segment 0, version V) while only writing an aggregate for rowset A—or provide no output tuples at all. The latter is exactly the shape used by RemovePreDeleteBitmapBatchesEachBlobKey: this loop removes the only source bitmap and returns OK. With a real bitmap that drops delete marks and can expose deleted rows. This is distinct from missing optional fields and out-of-range versions. Before any transaction can commit, validate the aligned output shape and require each destructive (rowset_id, segment_id) to bind to an eligible output at pre_rowset_agg_end_version; omit or explicitly handle empty-source stats, and add stats-only plus A/B negative tests.
mymeiyi
added a commit
to mymeiyi/doris
that referenced
this pull request
Aug 10, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66598 Problem Summary: Cloud cumulative compaction previously removed aggregated pre-rowset delete bitmaps with a Meta Service range clear. This could create a large FoundationDB transaction and remove keys outside the exact reported bitmap set. Collect each original bitmap by segment within the aggregation version range and let Meta Service remove the reported legacy and split blob keys individually in bounded transactions. Generate each split key on demand through blob_key so cleanup uses constant additional key memory instead of materializing every key in a vector. Requests without statistics retain range deletion for compatibility. A paginated delete bitmap read could also retain fragments from an expired transaction and append fragments from a replacement transaction after concurrent point cleanup, returning a gapped or truncated bitmap. Track the first blob key for the active bitmap, verify it in the replacement transaction, discard obsolete partial entries when it no longer exists, and validate blob sequence continuity. Suppress removal logs when no key is deleted. V2-only cumulative compaction skips the unused V1 pre-rowset aggregation before collecting source statistics. Meta Service also asserts that BE-produced cleanup statistic versions remain inside the compaction source interval. ### Release note Enable bounded, key-based removal of pre-rowset delete bitmap blobs by default during Cloud cumulative compaction. Prevent incomplete delete bitmaps from being returned when an FDB read transaction is replaced during concurrent point cleanup. V2-only writes skip unused V1 aggregation. Set enable_remove_pre_rowsets_delete_bitmap_by_keys=false to retain range deletion. ### Check List (For Author) - Test: Not run (per request); added BE and Meta Service unit coverage, including retained-tail and complete-removal snapshot retry interleavings - Behavior changed: Yes. Pre-rowset delete bitmap removal uses reported keys and bounded transactions by default; incomplete obsolete bitmap fragments are discarded after transaction replacement; V2-only writes skip unused V1 aggregation; empty removal logs are suppressed. - Does this need documentation: No
mymeiyi
force-pushed
the
delete-dmb-key
branch
from
August 10, 2026 12:25
b321913 to
be7481b
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29378 ms |
Contributor
TPC-DS: Total hot run time: 158905 ms |
Contributor
ClickBench: Total hot run time: 23.91 s |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#66598 Problem Summary: Cloud cumulative compaction previously removed aggregated pre-rowset delete bitmaps with a Meta Service range clear. This could create a large FoundationDB transaction and remove keys outside the exact reported bitmap set. Collect each original bitmap by segment within the aggregation version range and let Meta Service remove the reported legacy and split blob keys individually in bounded transactions. Generate each split key on demand through blob_key so cleanup uses constant additional key memory instead of materializing every key in a vector. Requests without statistics retain range deletion for compatibility. A paginated delete bitmap read could also retain fragments from an expired transaction and append fragments from a replacement transaction after concurrent point cleanup, returning a gapped or truncated bitmap. Track the first blob key for the active bitmap, verify it in the replacement transaction, discard obsolete partial entries when it no longer exists, and validate blob sequence continuity. Suppress removal logs when no key is deleted. V2-only cumulative compaction skips the unused V1 pre-rowset aggregation before collecting source statistics. Meta Service also asserts that BE-produced cleanup statistic versions remain inside the compaction source interval. The automatic-versionstamp blob writer also passed the value version and fragment sequence to blob_key in reverse order. This encoded the first fragment with version zero and made multi-fragment blobs invalid during range iteration. Pass the fragment sequence before the value version, consistent with the blob_key interface and the non-versioned writer. ### Release note Enable bounded, key-based removal of pre-rowset delete bitmap blobs by default during Cloud cumulative compaction. Prevent incomplete delete bitmaps from being returned when an FDB read transaction is replaced during concurrent point cleanup. V2-only writes skip unused V1 aggregation. Set enable_remove_pre_rowsets_delete_bitmap_by_keys=false to retain range deletion. ### Check List (For Author) - Test: Not run (per request); added BE and Meta Service unit coverage, including retained-tail and complete-removal snapshot retry interleavings. The blob argument-order fix was statically reviewed and git diff --check passed. - Behavior changed: Yes. Pre-rowset delete bitmap removal uses reported keys and bounded transactions by default; incomplete obsolete bitmap fragments are discarded after transaction replacement; V2-only writes skip unused V1 aggregation; empty removal logs are suppressed; automatic-versionstamp blobs encode the requested value version and increasing fragment sequence. - Does this need documentation: No
mymeiyi
force-pushed
the
delete-dmb-key
branch
from
August 11, 2026 02:20
be7481b to
d2c7631
Compare
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29430 ms |
Contributor
TPC-DS: Total hot run time: 158442 ms |
Contributor
ClickBench: Total hot run time: 23.98 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: Cloud cumulative compaction previously removed aggregated pre-rowset delete bitmaps with a Meta Service range clear, which may cause pressure to FDB.
Solution: This pr change to range remove to point remove to FDB with a be.conf:
enable_remove_pre_rowsets_delete_bitmap_by_keys=trueif
enable_remove_pre_rowsets_delete_bitmap_by_keys=false: