Repository navigation
[fix](cloud) Skip stale delete bitmap aggregation - #68084
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
🔵 Needs a closer look
Transaction and concurrency paths require final human review; tests were not executed.
Pull request overview
Fixes stale Cloud delete-bitmap aggregation by validating compaction output ranges before metadata updates.
Changes:
- Adds per-transaction output validation.
- Preserves error propagation across split transactions.
- Adds unit coverage and a two-BE regression test.
File summaries
| File | Description |
|---|---|
regression-test/suites/compaction/test_cloud_mow_compaction_agg_bitmap_two_be.groovy |
Adds a delayed two-BE aggregation race reproducer. |
cloud/test/meta_service_test.cpp |
Tests stale, malformed, concurrent, and split-transaction scenarios. |
cloud/src/meta-service/meta_service.cpp |
Validates output liveness and propagates transaction errors. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Approval opinion: approve. I found no blocking correctness issue in the exact-head static review of 4222d696c1d7601710abdc4a0b4ab4156d6a5ba5 against base df088085ca1956b63d739f0cf9600f90229852d6.
- Goal, scope, and tests: The change narrowly prevents a delayed cumulative-compaction pre-rowset aggregation from overwriting a newer one. It validates the live output range in the initial and every split meta-service transaction. Unit coverage exercises valid, obsolete, removed, malformed, in-flight conflict, write-split, cleanup-split, point/range, and idempotent cases; the two-BE Docker case reproduces the publication/aggregation race and cold-reads persisted state after restart.
- Concurrency and data correctness: Concurrent BEs are fenced through a non-snapshot FoundationDB read of the live rowset key. Publication before a checked transaction commits causes conflict; publication between split transactions is observed before the next mutation. Already-committed prefixes are safe because all aggregate writes precede cleanup and the superseding aggregation either overwrites them or can use the completed summary/source suffix. No new process locks or lock-order risks are introduced.
- Lifecycle, configuration, and performance: No production config, thread, static/global lifetime, ownership, or memory-tracking behavior changes. The added work is one small rowset read/parse per bounded transaction and is necessary for the conflict fence.
- Compatibility and parallel paths: No protobuf field, storage format, or symbol changes are introduced; the fix uses existing optional range fields. Delete-bitmap store versions 1 and 3 use the fenced v1 aggregation, while version 2 bypasses it. The sole production
-3RPC producer supplies the gate fields; schema-change and full-compaction paths remain distinct. Wire compatibility is preserved, though the protection is naturally effective only when the request reaches an upgraded meta-service. - Conditions, errors, persistence, and observability: The gate is specific to without-lock compaction aggregation (
lock_id=-3with both range fields). Obsolete work remains an intentional successful no-op, while read/parse/create/commit errors remain non-OK and conflicts retain retry behavior. Versioned metadata still atomically updates the ordinary live rowset key used by the fence. Existing identifiers in errors plus the new INFO skip log are sufficient for diagnosis; no FE EditLog or FE-BE variable propagation is involved. - Test-result review: The SQL-vs-SQL Docker assertions are ordered and need no handwritten
.out; added unit expectations are consistent with point/range cleanup semantics. Per the runner contract this review was static only: I did not build or run tests. Current PR checks show formatter/checkstyle/license/secret checks passing, while functional build/test jobs were skipped. - Additional user focus: No additional focus point was supplied.
All applicable code-review checkpoints were covered, all three initial risk questions were resolved with code evidence, and no unresolved candidate or duplicate inline concern remains.
### What problem does this PR solve? Problem Summary: Delayed compaction aggregation can overwrite newer delete bitmaps and make deleted rows visible. Validate the compaction output range in every MS transaction, stopping stale requests before writes or cleanup. Preserve non-OK error checks and add unit coverage for concurrent replacement, same-range replacement, and transaction splitting, plus a two-BE Docker case. ### Release note Fix deleted rows becoming visible after stale compaction bitmap aggregation. ### Check List (For Author) - Test: ASAN MetaServiceTest.* passed all 109 tests on the development machine. Clang-tidy, formatting, and diff checks passed. The Docker case reproduced the bug before the fix; it has not been rerun after the fix. - Behavior changed: Yes. Stale aggregation is skipped; errors stop processing. - Does this need documentation: No
4222d69 to
dede454
Compare
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Reviewed all three changed files at dede454. No substantiated new findings and no existing blocking inline comments. The complete production, test/lifecycle, and independent risk reviews all returned NO_NEW_VALUABLE_FINDINGS on the unchanged empty finding set. Converged in 1 round.
The change checks the current compaction output range in every update-delete-bitmap transaction. A delayed [4,5] aggregation stops when a newer [3,5] output has replaced it; replacement after the check conflicts with the non-snapshot read. The same guard is repeated after write and cleanup batch splits.
Critical checkpoint conclusions:
- Goal and correctness: the current-output check and request-wide stop propagation address stale aggregate overwrite and stale cleanup. Same-range output rewrites remain valid for unchanged pre-rowsets because the immutable version-interval deletion union is equivalent.
- Scope and reuse: one shared validity helper covers the required transaction boundaries; the boolean result distinguishes request termination from a per-source-rowset skip. Production changes and supporting tests are focused.
- Concurrency and locks: independent BE compactions and MS RPCs are fenced through FDB read/write conflicts in the mutating transaction. No new mutexes, heavy lock-held work, or lock-order/deadlock concerns were introduced.
- Lifecycle and initialization: each newly created transaction rechecks validity; early returns discard uncommitted work through existing ownership. No new threads, escaping references, globals, or static-initialization dependencies.
- Configuration: no production settings added or mutability behavior changed; the regression uses existing options.
- Compatibility: no new RPC fields, headers, key formats, or stored value formats. Compaction still clears and publishes current rowset keys with versioned metadata enabled. The real aggregation caller publishes its own current output before this RPC, including in a cloned instance; inherited historical metadata is not used to establish liveness.
- Parallel paths: both bitmap-write callers preserve false-result and non-OK checks. Initial, write-split, and point-cleanup-split transactions are guarded; range cleanup uses the guarded current transaction. Locked load/schema-change paths retain existing behavior, and pure bitmap-v2 aggregation remains bypassed.
- Conditions and errors: obsolete output is an intentional successful no-op that stops the entire request, including cleanup. Missing/replaced source rowsets remain individual skips. KV read/create/commit failures and malformed protobufs still report failure.
- Test coverage: statically checked valid/repeated, same-range, replaced, removed, malformed, post-read conflict, write-split and cleanup-split scenarios across point/range removal. Fixtures supply live output ranges. The Docker test uses real selector/barrier APIs to order two BE compactions and ends with both BEs restarted for a cold persisted-bitmap query.
- Test results: no generated expected-result files changed. The regression uses supported ordered SQL-to-SQL comparisons and drops its table before setup.
- Observability: obsolete-output logs identify instance, tablet and interval; failures include tablet/key context. Existing transaction statistics and failure counters remain suitable for this change.
- Persistence and failover: validity reads and mutations use ordinary FDB transactions; no FE EditLog changes. MS/BE retries execute the validity check again, including after ambiguous commits, and still-live replay uses deterministic replacement/cleanup.
- Data writes and crashes: aggregate writes precede source cleanup. Already committed prefixes are preserved; invalidation stops later transactions. A crash or stale stop can retain redundant source entries without letting subsequent obsolete cleanup erase needed deletion information.
- FE/BE variables: none added; the existing caller already supplies the actual output interval and source rowset versions.
- Performance: one required current-rowset point read per affected transaction, with no new scans or unbounded retry loop.
- Further issues and invariants: reviewed same-range rewrites, current/versioned/clone metadata, source-rowset replacement, partial batches, retry/error propagation and post-no-op cache prefill. No additional defect was substantiated in data/version/delete-bitmap consistency or ownership.
User focus: no additional focus was supplied; the entire PR was reviewed.
Validation limits: static inspection only, as required by this review environment. No builds, unit tests, Docker execution, FDB fault injection or runtime failover experiments were run. Versioned/clone and transient-retry behavior were traced in code rather than exercised by dedicated new tests. No unresolved review candidate or uncovered changed path remains.
Review converged after 1 round(s).
Problem Summary: Delayed compaction aggregation can overwrite newer delete bitmaps and make deleted rows visible. Validate the compaction output range in every MS transaction, stopping stale requests before writes or cleanup. Preserve non-OK error checks and add unit tests and a two-BE Docker reproducer.
…eplacement test (#69349) ### What problem does this PR solve? Issue Number: None Related PR: #68084, #68100 Problem Summary: **In short** `MetaServiceTest.GetDeleteBitmapReturnsCompleteReplacementAfterTxnTooOld` has failed on master since #68084 and #68100 were merged two minutes apart on 2026-10-20, so the Cloud UT pipeline is red for every PR. The test, added by #68100, injects a compaction aggregation without the output rowset that #68084 started to require, so the meta service skips the aggregation as obsolete. This PR adds that rowset to the test, the way #68084 did for the other aggregation tests. It changes one line of a test and no meta service code. **Background** - A compaction that aggregates the delete bitmaps of versions [start, end] sends `update_delete_bitmap` with `lock_id = -3`, `without_lock`, and `pre_rowset_agg_start_version` / `pre_rowset_agg_end_version`. - #68084 makes the meta service check, in every transaction, that the output rowset of such an aggregation still exists: the rowset meta at the end version must exist and cover [start, end]. Otherwise the request is treated as obsolete and returns OK without writing anything. #68084 added that output rowset (`put_delete_bitmap_test_rowset(..., 2, "r1-2", 1)`) to the existing tests that send such requests. - #68100 added `GetDeleteBitmapReturnsCompleteReplacementAfterTxnTooOld`. While `get_delete_bitmap` reads a bitmap across several transactions, a sync point injects an aggregation of [1, 2] and makes the read restart with `TXN_TOO_OLD`; the test checks that the restarted read returns the complete replacement. It was written against a master without #68084. **The problem, and what it cost** - With #68084, the injected aggregation has no output rowset at version 2, so `check_pre_rowset_aggregation` skips it. The restarted read returns the old three-chunk bitmap, the byte threshold stops it before `following_rowset`, and the first iteration fails: ``` cloud/test/meta_service_test.cpp:8501: Failure Expected equality of these values: get_res.rowset_ids_size() Which is: 3 expected_bitmap_count Which is: 4 Google Test trace: cloud/test/meta_service_test.cpp:8382: replacement_size=1 ``` - The ASSERT ends the test, so the second iteration (`replacement_size = 3 * DEFAULT_BLOB_SPLIT_SIZE`) never runs. `meta_service_test` exits non-zero, and since 2026-10-20 11:16 +08:00 the Cloud UT pipeline has failed for every PR, whatever the PR changes. - branch-4.2 already has the pick of #68100 (#69307). Once the pick of #68084 (#69306) is merged, branch-4.2 fails the same way. **How this PR fixes it** Add the output rowset of the injected aggregation to the test, as #68084 did for the other aggregation tests: ```cpp put_delete_bitmap_test_rowset(meta_service.get(), instance_id, tablet_id, 2, "r1-2", 1); ``` With it, the check passes, the replacement is written, and the test exercises the restarted read it was written for. Only that check reads the new rowset meta; `get_delete_bitmap` does not read rowset metas, so nothing else in the test changes. **Results** `run-cloud-ut.sh --run` in a Linux arm64 container (Debian 12, ldb toolchain v0.25, the linux-aarch64 prebuilt third-party tree, a local FDB 7.1.57): | Run, on master `33c460d7365` | `GetDeleteBitmapReturnsCompleteReplacementAfterTxnTooOld` | Other tests | |---|---|---| | without this PR (the added line removed) | fails as on the pipeline: 3 rowset ids instead of 4, under `replacement_size=1` | not run | | with this PR | passes | all 33 test binaries pass except `StopWatchTest.SimpleTest` | The same two runs on master `1d03523339f`, before the rebase, gave the same results. **Classes, and how they call each other** ``` MetaServiceTest.GetDeleteBitmapReturnsCompleteReplacementAfterTxnTooOld ├─ put_delete_bitmap_test_rowset(version 1, rowset_id) ├─ put_delete_bitmap_test_rowset(version 2, "r1-2", start 1) <- added └─ MetaServiceImpl::get_delete_bitmap └─ sync point "get_delete_bitmap_err" (in the middle of the read) ├─ MetaServiceImpl::update_delete_bitmap(lock_id -3, aggregation [1, 2]) │ └─ check_pre_rowset_aggregation: the rowset meta at version 2 must cover [1, 2] └─ TXN_TOO_OLD -> get_delete_bitmap restarts the read ``` Nothing outside `cloud/test/meta_service_test.cpp` changes. **Merge order** No dependency on master. On branch-4.2 this should go in together with #69306 (the pick of #68084), or right after it.
…eplacement test (#69349) ### What problem does this PR solve? Issue Number: None Related PR: #68084, #68100 Problem Summary: **In short** `MetaServiceTest.GetDeleteBitmapReturnsCompleteReplacementAfterTxnTooOld` has failed on master since #68084 and #68100 were merged two minutes apart on 2026-10-20, so the Cloud UT pipeline is red for every PR. The test, added by #68100, injects a compaction aggregation without the output rowset that #68084 started to require, so the meta service skips the aggregation as obsolete. This PR adds that rowset to the test, the way #68084 did for the other aggregation tests. It changes one line of a test and no meta service code. **Background** - A compaction that aggregates the delete bitmaps of versions [start, end] sends `update_delete_bitmap` with `lock_id = -3`, `without_lock`, and `pre_rowset_agg_start_version` / `pre_rowset_agg_end_version`. - #68084 makes the meta service check, in every transaction, that the output rowset of such an aggregation still exists: the rowset meta at the end version must exist and cover [start, end]. Otherwise the request is treated as obsolete and returns OK without writing anything. #68084 added that output rowset (`put_delete_bitmap_test_rowset(..., 2, "r1-2", 1)`) to the existing tests that send such requests. - #68100 added `GetDeleteBitmapReturnsCompleteReplacementAfterTxnTooOld`. While `get_delete_bitmap` reads a bitmap across several transactions, a sync point injects an aggregation of [1, 2] and makes the read restart with `TXN_TOO_OLD`; the test checks that the restarted read returns the complete replacement. It was written against a master without #68084. **The problem, and what it cost** - With #68084, the injected aggregation has no output rowset at version 2, so `check_pre_rowset_aggregation` skips it. The restarted read returns the old three-chunk bitmap, the byte threshold stops it before `following_rowset`, and the first iteration fails: ``` cloud/test/meta_service_test.cpp:8501: Failure Expected equality of these values: get_res.rowset_ids_size() Which is: 3 expected_bitmap_count Which is: 4 Google Test trace: cloud/test/meta_service_test.cpp:8382: replacement_size=1 ``` - The ASSERT ends the test, so the second iteration (`replacement_size = 3 * DEFAULT_BLOB_SPLIT_SIZE`) never runs. `meta_service_test` exits non-zero, and since 2026-10-20 11:16 +08:00 the Cloud UT pipeline has failed for every PR, whatever the PR changes. - branch-4.2 already has the pick of #68100 (#69307). Once the pick of #68084 (#69306) is merged, branch-4.2 fails the same way. **How this PR fixes it** Add the output rowset of the injected aggregation to the test, as #68084 did for the other aggregation tests: ```cpp put_delete_bitmap_test_rowset(meta_service.get(), instance_id, tablet_id, 2, "r1-2", 1); ``` With it, the check passes, the replacement is written, and the test exercises the restarted read it was written for. Only that check reads the new rowset meta; `get_delete_bitmap` does not read rowset metas, so nothing else in the test changes. **Results** `run-cloud-ut.sh --run` in a Linux arm64 container (Debian 12, ldb toolchain v0.25, the linux-aarch64 prebuilt third-party tree, a local FDB 7.1.57): | Run, on master `33c460d7365` | `GetDeleteBitmapReturnsCompleteReplacementAfterTxnTooOld` | Other tests | |---|---|---| | without this PR (the added line removed) | fails as on the pipeline: 3 rowset ids instead of 4, under `replacement_size=1` | not run | | with this PR | passes | all 33 test binaries pass except `StopWatchTest.SimpleTest` | The same two runs on master `1d03523339f`, before the rebase, gave the same results. **Classes, and how they call each other** ``` MetaServiceTest.GetDeleteBitmapReturnsCompleteReplacementAfterTxnTooOld ├─ put_delete_bitmap_test_rowset(version 1, rowset_id) ├─ put_delete_bitmap_test_rowset(version 2, "r1-2", start 1) <- added └─ MetaServiceImpl::get_delete_bitmap └─ sync point "get_delete_bitmap_err" (in the middle of the read) ├─ MetaServiceImpl::update_delete_bitmap(lock_id -3, aggregation [1, 2]) │ └─ check_pre_rowset_aggregation: the rowset meta at version 2 must cover [1, 2] └─ TXN_TOO_OLD -> get_delete_bitmap restarts the read ``` Nothing outside `cloud/test/meta_service_test.cpp` changes. **Merge order** No dependency on master. On branch-4.2 this should go in together with #69306 (the pick of #68084), or right after it.
…69306) Cherry-picked from #68084 Co-authored-by: meiyi <[email protected]>
Problem Summary: Delayed compaction aggregation can overwrite newer delete bitmaps and make deleted rows visible. Validate the compaction output range in every MS transaction, stopping stale requests before writes or cleanup. Preserve non-OK error checks and add unit tests and a two-BE Docker reproducer.