Skip to content

[fix](cloud) Skip stale delete bitmap aggregation - #68084

Merged
liaoxin01 merged 1 commit into
apache:masterfrom
mymeiyi:fix-mow-0916
Oct 20, 2026
Merged

liaoxin01 merged 1 commit into
apache:masterfrom
mymeiyi:fix-mow-0916

Conversation

@mymeiyi

@mymeiyi mymeiyi commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

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.

Copilot AI lite review requested due to automatic review settings September 16, 2026 11:02
@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?

@mymeiyi

mymeiyi commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

/review

Copilot AI 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.

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

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

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 -3 RPC 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=-3 with 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
@mymeiyi

mymeiyi commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

Cloud UT Coverage Report

Increment line coverage 81.69% (58/71) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 77.54% (2061/2658)
Line Coverage 65.85% (37913/57572)
Region Coverage 53.18% (35478/66710)
Branch Coverage 56.66% (11406/20130)

@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

@liaoxin01

Copy link
Copy Markdown
Contributor

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

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:

  1. 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.
  2. 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.
  3. 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.
  4. 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.
  5. Configuration: no production settings added or mutability behavior changed; the regression uses existing options.
  6. 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.
  7. 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.
  8. 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.
  9. 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.
  10. Test results: no generated expected-result files changed. The regression uses supported ordered SQL-to-SQL comparisons and drops its table before setup.
  11. 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.
  12. 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.
  13. 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.
  14. FE/BE variables: none added; the existing caller already supplies the actual output interval and source rowset versions.
  15. Performance: one required current-rowset point read per affected transaction, with no new scans or unbounded retry loop.
  16. 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).

@wyxxxcat wyxxxcat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@liaoxin01
liaoxin01 merged commit a60f34a into apache:master Oct 20, 2026
39 checks passed
github-actions Bot pushed a commit that referenced this pull request Oct 20, 2026
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.
morningman added a commit that referenced this pull request Oct 20, 2026
…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.
github-actions Bot pushed a commit that referenced this pull request Oct 21, 2026
…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.
yiguolei pushed a commit that referenced this pull request Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants