Repository navigation
[fix](cloud) Restart incomplete delete bitmap reads after transaction expiry - #68100
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
🔵 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 afterfollowing_rowsetis processed: the accumulated bytes arecompleted_bitmap + replacement_bitmap + v2_storage + completed_bitmap.get_delete_bitmapsetshas_moreonly when the threshold is reached before the final requested rowset (meta_service.cpp:4812-4823), so this case returns all three rowsets withhas_more=falseandreturned_rowset_ids_size()==3, contradicting the expectations below. Lower the threshold so it is reached afterrowset_idin 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.
|
/review |
There was a problem hiding this comment.
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 andhas_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) { |
There was a problem hiding this comment.
[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
b2ce920 to
71a363f
Compare
|
/review |
|
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. The selected account is excluded until 2026-09-19T08:24:00Z. Please trigger /review again; another configured account may be available. |
|
run buildall |
1 similar comment
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
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).
… 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.
… transaction expiry #68100 (#69307) Cherry-picked from #68100 Co-authored-by: meiyi <[email protected]>
…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.
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.