Skip to content

[fix](cloud) Restart incomplete delete bitmap reads after transaction expiry - #68100

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

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

Conversation

@mymeiyi

@mymeiyi mymeiyi commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Problem Summary: Retrying a paginated delete bitmap read after TXN_TOO_OLD could return truncated or mixed data if aggregation replaced the bitmap concurrently.
Restart only the unfinished bitmap and preserve completed results and byte counts.
Allow three retries per bitmap, resetting the budget only when reading advances to another bitmap.

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

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

The read_v2 test threshold conflicts with the expected pagination results.

Pull request overview

Updates delete-bitmap pagination retries after TXN_TOO_OLD, preserving completed results and byte counts.

Changes:

  • Restarts only the unfinished bitmap.
  • Preserves completed response data and retry budgets.
  • Adds replacement-race and retry-budget tests.
File summaries
File Summary
cloud/test/meta_service_test.cpp Adds concurrency and retry-budget coverage; the read_v2 threshold needs adjustment.
cloud/src/meta-service/meta_service.cpp Implements bitmap-scoped retry state and response rollback.
Review details

Suppressed comments (1)

cloud/test/meta_service_test.cpp:8118

  • For read_v2, this threshold is reached only after following_rowset is processed: the accumulated bytes are completed_bitmap + replacement_bitmap + v2_storage + completed_bitmap. get_delete_bitmap sets has_more only when the threshold is reached before the final requested rowset (meta_service.cpp:4812-4823), so this case returns all three rowsets with has_more=false and returned_rowset_ids_size()==3, contradicting the expectations below. Lower the threshold so it is reached after rowset_id in the v2 case.
        get_req.set_dbm_bytes_threshold(completed_bitmap.size() * 2 + replacement_size +
                                        (read_v2 ? v2_storage.ByteSizeLong() : 1));
  • Files reviewed: 2/2 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.

@mymeiyi

mymeiyi commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

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

Static-only review of head b2ce920c67e8a3bf246609f1a9e60db60c6e1656 against base 5d167701cac5218e97a0c20c940380eb59145443.

I found one P2 retry-scope issue (inline): after an exhausted bitmap finishes at a page boundary in a form whose completion is already known, the counter is not advanced until the next logical key is decoded. A continuation TXN_TOO_OLD can therefore force a whole-RPC failure/restart instead of giving the next bitmap its own budget.

Checkpoint conclusions:

  • Goal/tests: The rollback/restart fixes the partial old/new splice, and the added tests cover shrink and multi-chunk replacement, parallel arrays/counters, mixed v1/v2 batching, and basic retry-budget behavior. The exhausted-A/known-complete-boundary/continuation-expiry case is missing.
  • Scope: The change is focused on v1 pagination and its unit coverage.
  • Concurrency/lifecycle: State is RPC-local; expired transactions are accounted and released before fresh snapshots. No new locks, threads, globals, or lifecycle hazards were found.
  • Compatibility/parallel paths: Legacy and split v1 key forms were traced; v2 buffers locally and needs no equivalent response rollback. There is no wire, configuration, or storage-format change.
  • Correctness/persistence: The v1 arrays and byte/count checkpoints rewind together; prior rowsets, v2 data, and batching thresholds remain consistent. BE union semantics make preserved completed aggregation values safe. No write-path or persistence defect was found.
  • Error handling/performance/observability: Transaction errors remain checked and retry logging is adequate, but the inline boundary case can repeat expensive full-request work or fail when the outer retry is disabled.
  • Existing review context: The later suppressed threshold concern was rechecked; the mixed v1/v2 byte total reaches the configured >= threshold after the second rowset, matching the asserted two returned rowsets and has_more=true.
  • Tests: Static review only. Builds and tests were prohibited by the review contract, so the author-reported test execution was not independently rerun.

No additional user focus was provided.

{instance_id, tablet_id, rowset_ids[i], ver, seg_id});
// A successful page is not progress if a retry will reread the same
// bitmap. Reset the budget only when its logical start moves forward.
if (key != bitmap_start_key) {

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.

[P2] Advance the retry checkpoint after a known-complete bitmap

The retry counter is reset only after a key from the next (version, segment) is decoded. If A has already used its three retries and then a successful page ends with a bitmap that is provably complete (for example, a legacy unsuffixed value or a short final blob chunk), retry remains 3. A TXN_TOO_OLD on the continuation read then bypasses the retry branch before B is decoded, so B gets no internal budget and the caller may restart the whole RPC. Please advance the checkpoint/budget when completion is locally known, and add a regression with three A retries followed by an expiry on the first continuation toward B.

### What problem does this PR solve?

Issue Number: None
Related PR: apache#66598

Problem Summary: Retrying a paginated delete bitmap read after TXN_TOO_OLD could
return truncated or mixed data if aggregation replaced the bitmap concurrently.
Restart only unfinished bitmaps and preserve completed results and byte counts.
Allow three retries per checkpoint. Advance the checkpoint and reset its budget
when a bitmap is known to be complete, including at page boundaries.

### Release note

Fix incorrect delete bitmap results and premature retry failures after transaction expiry.

### Check List (For Author)

- Test: Unit Test. All 14 GetDeleteBitmap* tests passed with ASAN on the development machine; formatting and clang-tidy passed.
- Behavior changed: Yes. Reread unfinished bitmaps with bounded retries and preserve completed progress.
- Does this need documentation: No
@mymeiyi

mymeiyi commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

Copy link
Copy Markdown
Contributor

Codex automated review failed and did not complete.

Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Sep 19th, 2026 8:24 AM.
Workflow run: https://github.com/apache/doris/actions/runs/35177939906

The selected account is excluded until 2026-09-19T08:24:00Z. Please trigger /review again; another configured account may be available.

@mymeiyi

mymeiyi commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

run buildall

1 similar comment
@gavinchou

Copy link
Copy Markdown
Contributor

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

Cloud UT Coverage Report

Increment line coverage 100.00% (41/41) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 77.84% (2079/2671)
Line Coverage 66.10% (38237/57844)
Region Coverage 53.62% (35877/66915)
Branch Coverage 57.05% (11537/20223)

@liaoxin01

Copy link
Copy Markdown
Contributor

/review

@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

@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 PR #68100 at 71a363f048d501158dcd6b78a48dee745d9ed16d. No new substantiated findings. The static review converged in one round: the full-code, test, and separate risk reviewers all returned NO_NEW_VALUABLE_FINDINGS, and the main reviewer independently verified their dispositions.

The inspected flow was BE tablet synchronization -> MS proxy -> paginated bitmap reads -> rollback/replay on expiry -> tablet-compaction validation -> BE bitmap merge. Concurrent aggregation/replacement and point cleanup were traced through their actual write paths.

Critical checkpoint conclusions:

  • Goal and evidence: the implementation restarts the unfinished logical bitmap after transaction expiry, preserves completed results and byte counts, and bounds retries while its checkpoint is unchanged. The added deterministic tests exercise replacement, retry exhaustion/progress, and completion at a page boundary; they were inspected, not executed.
  • Scope and clarity: changes are confined to one production function and focused unit tests in the two supplied changed paths. Checkpoint state matches the logical bitmap boundary.
  • Data/version/delete-bitmap correctness: unfinished bytes are discarded before changing snapshots; response arrays and counters restore together. Requested version bounds and existing MoW sentinel/TEMP_VERSION handling are unchanged.
  • Concurrency and locks: concurrent aggregation can replace the same logical bitmap; its writer removes old fragments before atomically writing the replacement. New read state is request local. No locks, shared production variables, lock-order changes, or deadlock risk were introduced.
  • Lifecycle and memory safety: the page iterator is reset before replacing the transaction; decoder identity is reset before replay. New state has lexical ownership, with no static initialization dependency, reference cycle, or new BE memory owner.
  • Configuration: no configuration item was added; existing byte limits and store-version switches remain in use.
  • Compatibility: RPC signatures, protobufs, persisted formats, and key layout are unchanged. Legacy whole values and supported split values remain readable.
  • Parallel paths: v1 and mixed v1/v2 reads use the corrected loop. The v2 path already retrieves a complete value before publishing it and does not need incremental-response rollback. Non-cloud paths are unaffected.
  • Special conditions: terminal-chunk inference follows the writer's minimum/equal-split guarantees. Full equal-sized chunks conservatively remain replayable; the INT64_MAX suffix skips every fragment of a known-complete bitmap while preserving later segments/versions.
  • Test coverage: new cases check shrinking/growing replacement, earlier completed values, mixed-format byte thresholds, first-page/same-bitmap exhaustion, next-bitmap progress, and legacy/single/short/full page tails. Existing cleanup, expiry, outer-retry, v1/v2 and hard-byte-limit tests were also inspected. Hook/config cleanup is scoped and deterministic.
  • Test results: assertions match the inspected control flow. No regression result files changed; no runtime pass is claimed.
  • Observability: retry logging includes tablet, rowset, checkpoint, retry count and page round; existing error statuses and failure counters remain available.
  • Transactions, persistence and failover: this is a read-path change with no new durable writes or EditLog state. RPC retries clear partial responses, and final compaction-state validation still rejects expired rowsets before BE ingestion.
  • Writes and crash behavior: inspected writer replacement is atomic for each logical bitmap; failed reads return non-OK and their partial results are not applied. No new write protocol, held lock, or crash-persistent resource was introduced.
  • FE/BE values: no transmitted variable was added. Existing BE response-alignment checks and bitmap merging remain compatible.
  • Performance and other issues: retries reread only the unfinished bitmap and retain completed work; checkpoint comparison is bounded per logical bitmap. Empty ranges, removed/obsolete tails, same-version segment transitions, decoder reinitialization and error propagation were rechecked without finding another defect.

Existing thread 4032667117 is addressed by the known-complete-tail checkpoint/budget reset and its new regression test; it was not reposted. No applicable existing P0/P1 finding remains. No additional user focus was supplied.

Validation limits: static inspection only, as required; no builds, tests, FoundationDB expiry run, or BE/Roaring end-to-end execution. Dedicated tests do not separately combine retry with same-version segment progression, stale tails, transaction-creation failure, other read errors, or a lowered hard byte cap; those branches were statically reviewed without a concrete defect. No unresolved review candidate or required static coverage gap remains.

Review converged after 1 round(s).

@liaoxin01
liaoxin01 merged commit e1afc16 into apache:master Oct 20, 2026
38 checks passed
github-actions Bot pushed a commit that referenced this pull request Oct 20, 2026
… expiry (#68100)

Problem Summary: Retrying a paginated delete bitmap read after
TXN_TOO_OLD could return truncated or mixed data if aggregation replaced
the bitmap concurrently.
Restart only the unfinished bitmap and preserve completed results and
byte counts.
Allow three retries per bitmap, resetting the budget only when reading
advances to another bitmap.
yiguolei pushed a commit that referenced this pull request Oct 20, 2026
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.
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