[fix](cloud) Invalidate version caches on visible commit retries - #67813
Merged
luwei16 merged 7 commits intoSep 16, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: close apache#67099 Related PR: None Problem Summary: A cloud commit retry can return a VISIBLE transaction without partition versions after the original response was lost. FE then returns success while queries can still use stale partition caches. Refresh all partitions of txn_info.table_ids using the existing MS version query before returning success, including the MoW already-visible shortcut. Return refresh errors while keeping the callbacks consistent with the transaction's durable commit state. ### Release note Fix stale reads after cloud commit retries by refreshing local partition caches. ### Check List (For Author) - Test: Unit Test - 29 tests pass via run-fe-ut.sh (CloudGlobalTransactionMgrTest and CloudPartitionTest). - FE Checkstyle: 0 violations; git diff --check passes. - No cluster deployment or SQL regression run. - Behavior changed: Yes. VISIBLE retries missing version results refresh local partition caches synchronously; failed refreshes return an error. - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
luwei16
changed the title
### What problem does this PR solve? Issue Number: close apache#67099 Related PR: apache#67813 Problem Summary: Master and the visible-commit cache refresh fix add independent tests at the same location in CloudGlobalTransactionMgrTest. Merge master and retain both the exclusive transaction watermark test and all version-refresh tests. Preserve the original cache refresh patch and upstream production changes. ### Release note None ### Check List (For Author) - Test: Unit Test - 30 tests passed via run-fe-ut.sh (CloudGlobalTransactionMgrTest and CloudPartitionTest). - FE Checkstyle: zero violations; diff checks against merged master passed. - No local product packaging, cluster deployment or SQL regression run. - Behavior changed: No. Conflict resolution preserves both existing changes. - Does this need documentation: No
Contributor
Author
|
run buildall |
gavinchou
reviewed
Sep 10, 2026
| return; | ||
| } | ||
| List<CloudPartition> partitions = new ArrayList<>(); | ||
| for (long tableId : Sets.newHashSet(tableIds)) { |
Contributor
There was a problem hiding this comment.
why try to Sets.newHashSet(tableIds)?
Contributor
TPC-H: Total hot run time: 16864 ms |
Contributor
TPC-DS: Total hot run time: 81509 ms |
Contributor
ClickBench: Total hot run time: 14.55 s |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: the patch targets the reported visible-retry stale-cache bug, but six P1 correctness/lifecycle gaps remain in the recovery topology.
Critical checkpoint conclusions:
- Goal and proof: The local unit tests prove master-partition refresh, batching, dropped-table handling, a VISIBLE MoW precheck, and callback success state. They do not prove the full goal because table caches and peer FEs remain stale, failed repair lacks a retry owner, and lazy/2PC retry paths escape recovery.
- Scope and focus: The code change is localized to the FE transaction response path and its tests, but the required behavior spans Meta Service status transitions, BE reporting, peer FE synchronization, SQL/Routine Load callers, and cache readers. No additional user focus was supplied.
- Concurrency: The fallback installs partition versions without the table version-write locks used by the normal and peer-sync paths, so version-read-locked planners can observe a mixed multi-partition snapshot. Remote RPCs are outside catalog read locks and no distinct deadlock was found.
- Lifecycle and error handling: A refresh exception is exposed after durable visibility and success callbacks; synchronous callers can attempt an impossible abort, Routine Load cannot safely retry, and the BE report caller drops non-OK repair results. Lazy COMMITTED completion has no reliable later FE repair owner.
- Configuration: No configuration item is added. Reliance on the existing periodic synchronizer is not a correctness guarantee because it is configurable off and can race lazy phase one.
- Compatibility: No wire or storage format changes were introduced. Final 2PC already-visible responses are nevertheless rejected before the new recovery, so that parallel API remains behaviorally incompatible with idempotent retry.
- Parallel paths and conditions: Immediate, lazy-visible, subtransaction, forced-immediate Table Stream, direct FE, BE-report, MoW-precheck, and final-2PC paths were traced. The visible-only predicate and txn_info.table_ids coverage are sound for paths that reach it; final 2PC and lazy COMMITTED do not.
- Test coverage and results: The added FE tests are correct for their local assertions but omit table/peer propagation, latch-based reader concurrency, BE report retry, synchronous caller semantics, final 2PC, and COMMITTED-to-VISIBLE lazy completion. No local build or tests were run because the review runner instructions explicitly prohibit builds. Current PR checks show compile, FE UT, BE UT, Cloud UT, P0/cloud_p0/external/nonconcurrent regression, and performance passing; check_coverage_fe is failing, without enough evidence to classify it as a separate PR defect.
- Observability: The new failure paths log transaction context, but logging alone does not provide durable retry; no separate metrics defect was substantiated.
- Transaction, persistence, and data correctness: Meta Service durability itself is unchanged and no EditLog schema is added, but FE post-commit state is not installed atomically or consistently across cache consumers, and failure is misreported after durable success. Failover/status boundaries were included in the lazy and already-visible analysis.
- FE/BE variables: No new thrift/protobuf variable is added. The existing BE-to-FE report contract was checked end to end and exposes the dropped-error issue.
- Performance: The all-partition refresh is batched but can still issue multiple synchronous RPCs; no independent performance blocker was substantiated beyond the correctness/locking findings.
- Other: Catalog add/drop races and monotonic partition writes were checked and did not yield a separate bug; no security-sensitive behavior is changed.
Review completion: complete. The main pass plus two complete-review passes and one risk-focused pass converged in round 2; every candidate is accepted, duplicate-fenced, or dismissed with evidence.
| + "] is already aborted. abort reason: " + transactionState.getReason()); | ||
| } else if (transactionState.getTransactionStatus() == TransactionStatus.COMMITTED | ||
| || transactionState.getTransactionStatus() == TransactionStatus.VISIBLE) { | ||
| if (transactionState.getTransactionStatus() == TransactionStatus.VISIBLE) { |
Contributor
There was a problem hiding this comment.
[P1] Do not complete a lazy retry while it is only COMMITTED
A timed-out lazy commit can durably stop at COMMITTED while TxnLazyCommitTask is still responsible for publishing the partition versions. This branch now treats that state as completed, but refreshes only VISIBLE; its callers return success before executeCommitTxnRequest, afterCommitTxnResp, or success callbacks run. The lazy task later changes the transaction to VISIBLE without notifying FE, and the periodic syncer is not a safe fallback because it can be disabled or race the pending partitions after the phase-one table-version bump. Please wait/repoll through visibility and then perform the normal cache/callback completion, or install a durable repair owner before returning success.
| } | ||
| } | ||
| try { | ||
| for (List<CloudPartition> batch : Lists.partition(partitions, Config.cloud_get_version_task_batch_size)) { |
Contributor
There was a problem hiding this comment.
[P1] Install the refreshed snapshot under table version locks
getSnapshotVisibleVersionFromMs writes these CloudPartition caches one by one, but this new path does not take the affected tables' versionWriteLocks. Cached planners take versionReadLocks specifically while snapshotting partition versions, and both the normal commit path and peer-FE sync path use the matching write locks. A query can therefore interleave after P1 is advanced but before P2 and plan one atomically visible transaction as [new(P1), old(P2)]. Please fetch the remote values outside locks, then install all affected partition values under table version-write locks acquired in ID order.
| } | ||
| try { | ||
| for (List<CloudPartition> batch : Lists.partition(partitions, Config.cloud_get_version_task_batch_size)) { | ||
| CloudPartition.getSnapshotVisibleVersionFromMs(batch, false); |
Contributor
There was a problem hiding this comment.
[P1] Refresh the table cache and peer FEs too
This fallback only mutates the master FE's CloudPartition objects. Unlike the normal path above, it neither advances each OlapTable.cachedTableVersion nor calls CloudFEVersionSynchronizer.pushVersionAsync. With the default Long.MAX_VALUE table/partition cache TTLs, a warmed SQL cache can still validate against the old table version on the master, and follower/observer FEs can keep planning with their old partition versions until the periodic daemon runs. Thus an acknowledged visible retry can still serve stale results. Please recover the table versions and propagate the recovered partition/table state through the same synchronizer path as a normal commit response.
| stopWatch.start(); | ||
| try { | ||
| txnState = commitTxn(commitTxnRequest, transactionId, is2PC, tabletCommitInfos, tabletIds); | ||
| CommitTxnResponse response = commitTxn(commitTxnRequest, transactionId, is2PC); |
Contributor
There was a problem hiding this comment.
[P1] Recover already-visible final 2PC retries
For a final 2PC retry, Meta Service returns TXN_ALREADY_VISIBLE plus only txn_info, but commitTxn(..., is2PC=true) rejects that status before it can return this response to afterCommitTxnResp. The apparent MoW precheck does not cover this: commitTransaction2PC calls getMowTableList(tableList, null), which always returns an empty list. Thus losing the first final-commit response leaves every retry failing with stale caches. Please treat the already-visible final-2PC response as the durable idempotent outcome and run the same cache recovery before completing it.
| MetricRepo.COUNTER_TXN_SUCCESS.increase(1L); | ||
| MetricRepo.HISTO_TXN_EXEC_LATENCY.update(txnState.getCommitTime() - txnState.getPrepareTime()); | ||
| } | ||
| afterCommitTxnResp(response, tabletCommitInfos, tabletIds); |
Contributor
There was a problem hiding this comment.
[P1] Do not return a normal commit failure after success callbacks
At this point Meta Service has returned the durable visible transaction and txnOperated is already true, so finally runs afterCommitted/afterVisible; nevertheless a refresh exception escapes from this call. TransactionEntry.commitTransaction interprets that as a failed commit, attempts to abort the already-visible transaction, and reports failure. Routine Load is worse: afterVisible renews/removes the old txn task, so retrying the returned error fails in beforeCommitted before cache repair can run. Please make repair independent/retryable (or invalidate caches) while returning an outcome that callers cannot route to abort or replay callbacks.
| } | ||
| } catch (UserException e) { | ||
| LOG.warn("failed to refresh versions for reported commit, txnId: {}", request.getTxnId(), e); | ||
| return new TStatus(TStatusCode.INTERNAL_ERROR).setErrorMsgs(Collections.singletonList(e.getMessage())); |
Contributor
There was a problem hiding this comment.
[P1] Give this failed refresh a retry owner
The only production caller of this report is send_stats_to_fe_async in be/src/cloud/cloud_meta_mgr.cpp; it converts this INTERNAL_ERROR to a failed status, logs it, and then deliberately returns Status::OK() without retrying. If an already-visible response reaches FE while VersionHelper exhausts its bounded retries, BE has already acknowledged the commit and this cache repair is never replayed, so the old version can remain cached after Meta Service recovers. Please retry/schedule the repair in FE or make the BE report path retry non-OK results; returning an error that its sole caller drops does not close the stale-read path.
### What problem does this PR solve? Issue Number: close apache#67099 Related PR: apache#67813 Problem Summary: Remove the extra HashSet conversion when refreshing partition versions for visible commit retries. Iterate the supplied transaction table IDs directly and remove the artificial duplicate ID from the existing two-table test. Retain all version, timestamp, TSO, response and RPC-count assertions. ### Release note None ### Check List (For Author) - Test: Static check - FE Checkstyle passed with zero violations; git diff --check passed. - Existing unit test fixture updated; no local unit tests rerun for this two-line change. - Full CI buildall will be requested after pushing this commit. - Behavior changed: Yes. Iterate supplied table IDs without deduplication. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 17075 ms |
Contributor
TPC-DS: Total hot run time: 83304 ms |
Contributor
ClickBench: Total hot run time: 14.8 s |
gavinchou
previously approved these changes
Sep 14, 2026
### What problem does this PR solve? Issue Number: close apache#67099 Related PR: apache#67813 Problem Summary: A cloud commit retry may return a VISIBLE transaction without version results. Invalidate the affected table and partition caches and notify peer FEs through the existing version RPC, so subsequent reads fetch fresh versions without making a durable commit fail on a version-service outage. Protect batch cache publication with version write locks and use in-memory epochs to prevent older RPC completions from clearing invalidation. ### Release note Prevent stale reads and false commit failures after cloud commit retries. ### Check List (For Author) - Test: Unit Test - 55 FE tests passed via run-fe-ut.sh (CloudGlobalTransactionMgrTest, CloudPartitionTest, OlapTableTest); FE Checkstyle: 0 violations; git diff --check passed. - Behavior changed: Yes. Missing-version VISIBLE responses invalidate caches; successful commit callbacks and return semantics are preserved. - Does this need documentation: No
luwei16
changed the title
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16997 ms |
Contributor
TPC-DS: Total hot run time: 82954 ms |
Contributor
ClickBench: Total hot run time: 14.79 s |
### What problem does this PR solve? Issue Number: close apache#67099 Related PR: apache#67813 Problem Summary: CI merges the PR into current master, where batch version reads call getVersionFromMeta(request, maxAttempts). The cache invalidation tests only stub the single-argument overload, causing four null-response errors. Merge master and route both overloads through the same scoped test stub, retaining the response injection, call counts and concurrency assertions. ### Release note None ### Check List (For Author) - Test: Unit Test - Reproduced the four CI errors after merging master. - All 58 focused FE tests passed in ordinary and JaCoCo coverage modes. - FE Checkstyle: zero violations; PR diff whitespace check passed. - Behavior changed: No (test fixture adaptation only) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: close apache#67099 Related PR: apache#67813 Problem Summary: The Maven build cache restores BE Java module jars without target/lib. Cached builds skip the runtime dependency copy, so a fresh workspace packages incomplete plugins and fails the plugin layout check. Always rerun the paired dependency cleanup and copy executions, including hadoop-deps's copy step, while retaining compiled-artifact cache reuse. ### Release note Fix incomplete Java plugin packages produced by cached builds. ### Check List (For Author) - Test: Manual test - Reproduced all 352 CI layout violations with actual Maven cache hits. - Normal and cached Java-extension builds produced matching runtime dependency manifests for eight plugins and hadoop-deps. - All nine cached modules removed obsolete dependency jar probes. - All eight plugins passed the existing layout checker; Checkstyle and git diff --check passed. - Behavior changed: Yes (cached builds regenerate runtime dependencies) - Does this need documentation: No
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: close apache#67099 Related PR: apache#67813, apache#68000 Problem Summary: The performance pipeline fails before benchmarks because Maven cache hits restore Java plugin jars without their runtime dependencies. Merge upstream master and adopt its equivalent cache cleanup/copy fix, resolving conflicts with the local repair and removing duplicate changes from this PR. Preserve the cloud version-cache invalidation changes. ### Release note None ### Check List (For Author) - Test: Manual test - build.sh --be-java-extensions passed after the merge. - All eight plugins and hadoop-deps hit the cache, regenerated the same runtime dependency manifests as an uncached build, and removed stale jars. - All eight plugins passed the existing layout checker; Checkstyle for the nine modules and PR diff whitespace checks passed. - Behavior changed: No (adopt the equivalent upstream build repair) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 16767 ms |
Contributor
TPC-DS: Total hot run time: 81778 ms |
Contributor
ClickBench: Total hot run time: 14.65 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
gavinchou
approved these changes
Sep 16, 2026
Contributor
There was a problem hiding this comment.
Request changes on exact head ac106fb1dd5d0a2bb6a05545918620158eb1c6d2 (static review only). The patch improves lost-visible-response recovery, but three cache-publication defects remain: a partial batch can return a mixed pre/post-commit partition snapshot; a batch can return a stale value after observing a concurrent refresher's epoch acknowledgement; and the periodic daemon cannot close the new table invalidation epoch, causing repeated Meta Service work and leaving paired partition invalidations pending on the equal-version path.
Critical checkpoint conclusions:
- Goal and correctness: The intended lost-
VISIBLEretry repair is not complete because the two batch-reader races can still plan stale or impossible snapshots after invalidation. - Scope and design: The five-file FE-only change is focused, but the epoch contract is not applied consistently across selective batch reads, concurrent single-item publishers, and the daemon's raw refresh path.
- Concurrency and locking: Version-lock sets are acquired in table-ID order and no Meta Service RPC is performed while holding them; no deadlock survived review. The remaining defects are snapshot/publication-order races described inline.
- Lifecycle, configuration, and compatibility: The new epochs are transient cache state and restart initializes uncached state safely. No configuration, persistent format, or FE/BE protocol field is added. Mixed-version, disabled-repair, and peer-FE recovery concerns are already covered by existing review threads and are not duplicated here.
- Transactions and parallel paths: Ordinary, lazy, MoW, final-2PC, subtransaction, and BE-report paths were traced, including
txn_info.table_idscompleteness. Existing lazy/2PC/callback/report gaps remain covered by existing threads; no duplicate comment is added. - Error handling and observability: RPC failures remain visible/retryable in the cache-aware readers, and logging is adequate for the changed paths. The daemon convergence defect creates repeated RPCs but no separate logging issue.
- Performance: The daemon's unacknowledged epoch can trigger an MS table-version RPC on every eligible pass and on cache-aware consumers until foreground repair. No other independent performance blocker survived review.
- Testing: Current PR checks are green, but the tests omit deterministic partial-cache invalidation, value-before-epoch publication, and daemon-convergence interleavings. Per the review runner contract, no local builds or tests were run.
- User focus: No additional focus was supplied.
Review completion: complete. The main pass plus three duplicate-safe rounds of normal and risk-focused review converged in round 3 with no new valuable findings; all candidates are accepted, duplicate-fenced, or dismissed with evidence.
| } | ||
|
|
||
| @VisibleForTesting | ||
| protected boolean isCachedVersionExpired() { |
Contributor
There was a problem hiding this comment.
[P1] Read the cached value after validating its epoch
The batch readers currently load the numeric cache before calling this epoch check. After invalidation leaves epoch 1/refreshed 0 and value v2, a concurrent single-item getVisibleVersion() can fetch v3, write it, and publish refreshed epoch 1 between those two operations. The batch then sees this check pass but returns its earlier local v2; ScanNode.setVisibleVersionForOlapScanNodes uses that stale value directly even for a one-partition query. This is distinct from the partial-subset race: the epoch never changes, so a later epoch recheck would still pass. In both partition and table batch readers, establish that the epoch is valid first and only then load the cached value (or make every epoch-acknowledging publisher take the matching version write lock), and add a deterministic interleaving test.
| Long version = versions.get(i); | ||
| if (version > Partition.PARTITION_INIT_VERSION) { | ||
| // For compatibility, the existing partitions may not have mtime | ||
| long mTime = versions.size() == versionUpdateTimesMs.size() ? versionUpdateTimesMs.get(i) : 0; |
Contributor
There was a problem hiding this comment.
[P1] Recheck cached siblings after a partial refresh
This write lock protects only the partitions passed in the expired subset, but the caller may already have copied other siblings from cache and released its read lock. For example, P1 is expired and P2 is cached at v2; after that snapshot, a VISIBLE retry invalidates both epochs, then this one-partition RPC returns P1=v3. The caller splices [v3, v2], and ScanNode.setVisibleVersionForOlapScanNodes sends that impossible mixed snapshot to the BEs even though the transaction made both partitions visible atomically. This is distinct from the prior unlocked-writer thread because the omitted sibling is outside this new lock. Fetch the full requested batch when any entry expires, or recheck every copied epoch and retry after intervening invalidation; apply the same rule to the partial table-version batch.
| @@ -3605,13 +3609,18 @@ protected boolean isCachedTableVersionExpired() { | |||
|
|
|||
| public boolean isCachedTableVersionExpired(long expirationMs) { | |||
| // -1 means no cache yet, need to fetch from MS | |||
Contributor
There was a problem hiding this comment.
[P2] Let the daemon complete the invalidation epoch
Once this predicate sees an epoch mismatch, CloudSyncVersionDaemon cannot make it false: the daemon reads authoritative table versions through raw getVisibleVersionFromMeta, then calls only setCachedTableVersion, which never advances refreshedTableVersionCacheEpoch. The table is therefore selected for an MS RPC on every later daemon pass (and by every cache-aware table-version consumer until a foreground wrapper repairs it). If a delayed normal notification already supplied the same numeric table version, the daemon also skips partition refresh, leaving all partition epochs mismatched. Capture each epoch before the daemon RPC, refresh invalidated partitions even when the numeric table version is unchanged, and acknowledge only that captured epoch after the paired refresh succeeds so a concurrent invalidation remains pending.
Contributor
Author
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#67813
commit: ac106fb1dd5d0a2bb6a05545918620158eb1c6d2
base: 1a3b141824bf4c8a0d77b95338a6978360282f24
reviewed_at: 2026-09-16T12:40+08:00
reviewer: luwei16
model: gpt-6-astra
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
morningman
added a commit
that referenced
this pull request
Sep 16, 2026
…on PRs from master in merge order (#65805 #67813) (#68073) Cherry-picked from #65805, #67813 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each created with `git cherry-pick -x` so the message ends with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830 and #68017. | # | Master commit | PR | Title | |---|---|---|---| | 1 | ec886f3 | #65805 | [fix](nereids) Disambiguate NULL/OFFSET metadata from same-named nested fields | | 2 | 3dbe4d3 | #67813 | [fix](cloud) Invalidate version caches on visible commit retries | Not included on purpose: - The 17 labelled PRs that already carry `incremental-computation-picked` (#62606 in the fork point, #67508 via #67712, the nine of #67830, the six of #68017). - #68012 carries the label but is a PR against this branch itself (merged as `6f7c87fa892`); nothing to pick. - #67820 is still open on master; this branch already carries its content via #67861. ### Prerequisite check - **#65805** lists #65591 and #66380 as related PRs: #65591 is still open and #66380 was closed unmerged, both superseded by #65805 itself. The nested-column-pruning series it builds on (#59263, #61888, #64535) is before the fork point, so it is already on this branch. No master commit between the fork point and #65805 touches `column_reader.{cpp,h}`, `descriptors.cpp`, the nereids pruning rules or the BE tests. The only overlap is the unrelated #66761 (TIMESTAMP_NS), which touches `Descriptors.thrift` (a different struct) and adds an unrelated test plus its `Config` import to `DescriptorToThriftConverterTest.java`; that caused the one conflict, see below. - **#67813** declares no related PR (closes #67099). Its behavior does not depend on any master commit missing here. The one unlabelled master commit that touches the same main files, #66296 ("Reduce cloud version sync config"), only adds `maxAttempts` overloads that none of #67813's main-code hunks use. The other drift in `CloudGlobalTransactionMgr.java` / `CloudGlobalTransactionMgrTest.java` is this branch's own #67861 (the branch-side version of the still-open #67820), which lives in `commitTxn` / `releaseFinishedTso` / `afterAbortTxnResp` and does not intersect the two hunks #67813 adds (`checkTransactionStateBeforeCommit` and the empty-partition-list branch of the commit response handling). Neither #66761 nor #66296 was picked; three mechanical adaptations were needed instead, each recorded in the pick's commit message: - **#65805** conflicted only in the import block of `DescriptorToThriftConverterTest.java` (the master hunk sits next to the `Config` import that #66761 added). Resolved by adding only `import org.apache.doris.thrift.DescriptorsConstants;`. Everything else is byte-identical to the master commit. - **#67813** conflicted only in the import block of `CloudGlobalTransactionMgrTest.java`: the branch already has `import org.apache.doris.rpc.RpcException;` through #67861, so that line became context; the other ten imports were taken as-is. - **#67813**, test-only: the master helper `mockVersionHelper()` stubs `VersionHelper.getVersionFromMeta(request, maxAttempts)`, an overload that only exists on master because of #66296, so `fe-core` test compilation failed (`method getVersionFromMeta ... cannot be applied to given types`). On this branch every read goes through the single-argument overload, so the helper now just returns `Mockito.mockStatic(VersionHelper.class)`. No main-code hunk of #67813 uses the `maxAttempts` overloads. ### Drift check against master For each pick, the diff of the touched files against the master commit's parent before the pick and against the master commit after the pick have identical `+`/`-` lines (only the import context lines differ as described above), i.e. each pick added exactly its master hunks. Leftover differences to master after the picks are: - #65805's files: `DescriptorToThriftConverterTest.java` and `Descriptors.thrift` differ from master by exactly #66761's hunks. - #67813's files: `OlapTable.java`, `CloudPartition.java`, `CloudGlobalTransactionMgr.java` and `CloudGlobalTransactionMgrTest.java`. Applying #66296 forward and #67861 in reverse in a temporary index brings the four main files to zero diff against master `3dbe4d3ca53`; the test file's remaining difference is the `RpcException` import overlap plus the `mockVersionHelper()` adaptation. `CloudFEVersionSynchronizer.java` is byte-identical to master. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main + test) with the test classes touched by the picks plus the two related cloud version-cache classes: 5 classes, 139 tests, 0 failures, 0 errors, BUILD SUCCESS (2:12 min) — `PruneNestedColumnTest` 61, `DescriptorToThriftConverterTest` 21, `CloudGlobalTransactionMgrTest` 37 (all 13 tests #67813 adds included), `CloudPartitionTest` 3, `OlapTableTest` 17. (`VersionHelperTest` from #67813's checklist does not exist on this branch; #66296 added it.) - FE checkstyle on fe-core: 0 violations. - BE: `-fsyntax-only` with the flags of the Release build (`compile_commands.json`, regenerated `gen_cpp` headers) passes for `storage/segment/column_reader.cpp`, `runtime/descriptors.cpp`, and with `-DBE_TEST -fno-access-control` for `test/storage/segment/column_reader_test.cpp` and `test/runtime/descriptor_test.cpp`. - The three touched groovy suites (`lambda_null_pruning`, `left_join_not_null_column`, `null_column_pruning`) parse cleanly (groovy parser check). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: minghong <zhouminghong@selectdb.com> Co-authored-by: Luwei <814383175@qq.com>
7 tasks
bobhan1
added a commit
to bobhan1/doris
that referenced
this pull request
Sep 17, 2026
### What problem does this PR solve? Problem Summary: Commit retries can race concurrent lazy publication. Stale temporary-rowset candidates may be published again, and an incomplete response may leave FE version caches partially updated. Preserve the non-snapshot batch read of all candidate tmp keys in the actual immediate write transaction. A missing key rejects the whole attempt with KV_TXN_CONFLICT; a later concurrent deletion creates a real FDB commit conflict. FE sends distinct tablet and partition counts for the whole load, including non-base indexes and the union of sub-transactions. Unknown counts are omitted. At each MS commit path's successful response, compare the processed tablet count and returned partition count with the supplied expectations. On mismatch, clear partition versions, table stats and version timestamps while preserving the successful transaction result. The empty response uses the cache invalidation already provided by apache#67813, without another commit-time version RPC. ### Release note Prevent duplicate rowset publication and stale FE version caches when commit retries race lazy publication. ### Check List (For Author) - Test: Unit Test; ASAN with real FoundationDB for the six focused cases; the complete lazy-commit suite passes 33 active tests with one pre-existing disabled test. The full-suite run preceded the snapshot-pagination helper adjustment; all six affected cases pass with the final helper. FE transaction and version-cache suites pass 60 tests. Checkstyle, clang-format 16 and git diff --check pass. No live cluster or SQL regression run. - Behavior changed: Yes; reject stale candidates and omit incomplete version results so FE invalidates its caches. - Does this need documentation: No
morningman
added a commit
that referenced
this pull request
Sep 20, 2026
…on PRs from master in merge order (#67820 #68088) (#68236) Cherry-picked from #67820, #68088 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each ending with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830, #68017, #68073 and #68151. | # | Master commit | PR | Title | |---|---|---|---| | 1 | 44e3ae2 | #67820 | [fix](binlog) Track committed TSO and fence uncertain commits for bounded incremental reads | | 2 | 3de3a75 | #68088 | [fix](binlog) Require SELECT privilege for binlog TVF | Not included on purpose: - The 25 labelled PRs that already carry `incremental-computation-picked`. - #68012 carries the label but is a PR against this branch itself (merged as `6f7c87fa892`); nothing to pick. ### How #67820 was picked This branch already carried #67820 through #67861, which backported the PR's first seven commits (up to `17272039558`) before the PR was merged. Between that cut and the merge the PR gained six more commits and five master merges, and the final design differs from what #67861 brought: the Meta Service recovery RPC (`get_tso_recovery_transactions`) and the FE startup/periodic recovery scan are gone, replaced by a durable per-instance commit-TSO fence (`txn_tso_fence_key`, `advance_tso_fence`, commit-time `TXN_COMMIT_TSO_EXPIRED` check, `enable_check_commit_tso_fence`), plus the review-feedback and "release maybe-committed TSO" fixes. A plain `cherry-pick -x 44e3ae2` conflicts in 23 files because the branch holds the intermediate design, so commit 1 was built by replaying, on top of the branch, exactly what the PR gained after the cut: - the PR's later commits `d16e67197b2`, `e53291d196b`, `864546d5531`, `8c55eb086e9`, `8d264ce294a`, `fc4f9969202`; - the content that landed inside its master merges, identified by diffing each merge against its `git merge-tree` automerge result: `c491a293426` (`setEnvTSOService` replaced by `Mockito.doReturn(tsoService).when(masterEnv).getTSOService()` — `masterEnv` is the same Mockito delegating mock here since #67813) and `29f133584b5` (the extra `TSOTransactionTrackerTest` coverage and two comment removals in `DorisFlightSqlProducer`). The other three merges only resolved import blocks against master-only code. The replay then got squashed into one commit with the master PR's message, the original author, and the `(cherry picked from commit 44e3ae2)` trailer. Adaptations, all recorded in the commit message: - `DorisFlightSqlProducer` / `DorisFlightSqlProducerTest` stay under `service/arrowflight` (the branch lacks the package move of #67866). - `8d264ce294a` is a no-op here: it removes a `catch (FlightRuntimeException)` block that #67883 added on master and this branch never had. - The branch-only `get_tso_recovery_transactions` RPC, its recovery scan, the five `TsoRecovery*` Meta Service tests and the NOLINT suppressions #67861 had added for them are removed, as on master. - The round-3 `mockVersionHelper()` adaptation in `CloudGlobalTransactionMgrTest` (no `VersionHelper.getVersionFromMeta(req, maxAttempts)` overload here, #66296) is kept. ### Prerequisite check - **#67820** declares #67181 and #67594 as related; #67181 (`e5a4e725fac`) is before the fork point and #67594 came with #67830. The rest of what the commit touches on master is import-block and neighbouring-code drift from unlabelled commits (#67866 / #67883 / #67966 Arrow Flight and session refactors, #67761 `get_prepare_txn_by_coordinator`, #66598 pre-rowset delete bitmaps, repair-tablet-index changes); none of it is used by the fence/committed-TSO logic. - **#68088** declares no related PR. The hook it implements (`TableValuedFunctionIf.checkAuth`) and the caller chain (`CheckPrivileges.visitLogicalTVFRelation` → `TableValuedFunction.checkAuth` → catalog function) are byte-identical between this branch and master, and the five-argument `AccessControllerManager.checkTblPriv` overload exists. Applied cleanly. ### Drift check against master - **#67820**: every one of the 54 files the master commit touches now contains the pick's content — the master commit reverse-applies cleanly per file onto this branch (50 files), and the four files where only the surrounding context differs (`config.h`, `meta_service_txn.cpp`, `StmtExecutor.java`, `StmtExecutorTest.java`) contain every added line and none of the removed ones. 33 of the 54 files are byte-identical to master at `44e3ae2b951`, including all of `fe/.../tso/`, `CloudGlobalTransactionMgr.java`, `MetaServiceProxy.java`, `keys.{h,cpp}`, `meta_service.h` and the regression suite/output. The remaining differences are unrelated master-only or branch-only code from unlabelled commits (#67761, #66598, #67866/#67883/#67966, repair-tablet-index, meta-cache columns, recycler configs) plus the documented `mockVersionHelper()` adaptation; no line in the committed-TSO/fence domain is left over from #67861. - **#68088**: all three files are byte-identical to master at `3de3a756f74`. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main + test) with every test class the picks touch: 12 classes, 175 tests, 0 failures, 0 errors, BUILD SUCCESS — `TSOServiceTest` 38, `CloudGlobalTransactionMgrTest` 37, `StmtExecutorTest` 27, `MetaServiceProxyTest` 22, `OlapScanNodeTest` 12, `TimeBasedChangeVisibleWaiterTest` 12, `TSOTransactionTrackerTest` 9 (the four tests added inside the PR's last master merge included), `DorisFlightSqlProducerTest` 7, `TsoStatusMetadataGeneratorTest` 5, `CloudCommittedTsoTest` 3, `SchemaTableTest` 2, `TableBinlogFunctionAuthTest` 1 (#68088). The `@Test` counts of the touched classes equal master's. - FE checkstyle on fe-core: 0 violations. - Meta Service: `-fsyntax-only` with the flags of `cloud/CMakeLists.txt` (`-Wall -Werror`, regenerated `gen_cpp/cloud.pb.h` with `AdvanceTsoFence*` / `TxnTsoFencePB` and without `GetTsoRecoveryTransactions*`) on `meta_service_txn.cpp`, `keys.cpp`, `http_encode_key.cpp`, `bvars.cpp` and, with `-DUNIT_TEST -DBE_TEST -fno-access-control`, on `meta_service_test.cpp`, `keys_test.cpp`, `http_encode_key_test.cpp`, `meta_service_helper_test.cpp`, `txn_lazy_commit_test.cpp`: no diagnostic in any line the pick touches (the only errors are the pre-existing macOS-only `pthread_setname_np` / `int64_t`-vs-`long` ones in untouched 2024/2025 code). clang-format 16 is clean on all 14 touched cloud files. - No BE file changes in this round (the BE side of #67820 was already byte-identical to master via #67861). - The new `auth_p0/test_binlog_tvf_auth.groovy` and the two #67820 suites parse cleanly (groovy parser check). --------- Co-authored-by: Luwei <814383175@qq.com> Co-authored-by: morrySnow <zhangwenxin@selectdb.com>
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
5 participants
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.
What problem does this PR solve?
Issue Number: close #67099
Related PR: None
Problem Summary: After a cloud commit response is lost, a retry can return a VISIBLE transaction without version results. FE can then keep using stale partition versions and the table version used to validate SQL caches.
Invalidate the affected table and partition version caches, including the MoW already-visible shortcut, and notify other FEs through the existing version synchronization RPC. Subsequent reads fetch fresh versions from MS. Commit processing performs no extra MS version query, so a version-service outage does not turn an already committed transaction into a commit error or require replaying its callbacks.
Publish batch cache updates under the tables' version write locks. Invalidation epochs prevent old in-flight MS responses and delayed commit notifications from restoring stale caches. The synchronization RPC uses table version -1 for invalidation; older FEs ignore it and retain their existing periodic synchronization. Peer notification keeps its existing asynchronous behavior and configuration.
Release note
Prevent stale reads and false commit failures after cloud commit retries.
Check List (For Author)