Repository navigation
[fix](cloud) Fence MetaService before giving up the TSO of a commit that failed with KV_TXN_COMMIT_ERR - #69925
Open
morningman wants to merge 1 commit into
Open
morningman wants to merge 1 commit into
morningman wants to merge 1 commit into
Conversation
…hat failed with KV_TXN_COMMIT_ERR ### What problem does this PR solve? Issue Number: None Related PR: apache#67820, apache#69831 Problem Summary: In cloud mode the Master FE registers the commit TSO of each transaction it sends to MetaService and keeps a committed TSO watermark below the oldest registered one. A bounded `@incr` read admits a window that ends at or below the watermark without waiting, on the guarantee that no commit with a TSO at or below it can still land. apache#67820 keeps that guarantee by giving up a TSO only once the outcome of its commit is settled: a success, an already visible or an already aborted answer finishes it; a failed RPC fences MetaService at the TSO first (`fenceAndAbandonCommitTso`), as commit_txn reads the fence in the FDB transaction it commits, so the commit has landed before the fence or is rejected by it; any other answer gives the TSO up at once, as a commit that failed for good. `KV_TXN_COMMIT_ERR` is not always such an answer. MetaService gives each FDB transaction a timeout (`fdb_txn_timeout_ms`, 10 seconds by default) and answers `KV_TXN_COMMIT_ERR` when the commit of the transaction timed out (FDB errors 1004 and 1031) or failed with an error it does not classify. A timeout cancels the FDB transaction, and a commit cancelled after it was sent may have happened or may still happen. FE gave such a TSO up without a fence, so the watermark could pass a commit that lands later, and a bounded `@incr` read admitted at that watermark would miss its rows. `KV_TXN_COMMIT_ERR` now takes the path of a failed RPC: MetaService is fenced at the TSO before it is given up. A fence that fails deactivates the TSO service, as on a failed RPC, so no read is admitted on the watermark until a calibration fences MetaService above every TSO given out before. The commit fails as before, with the fence error when the fence fails; the answer of MetaService is logged first then. | Answer of MetaService to commit_txn | Before | After | | --- | --- | --- | | `KV_TXN_COMMIT_ERR` | TSO given up | MetaService fenced at the TSO, then the TSO given up | | Failed RPC | MetaService fenced at the TSO, then the TSO given up | Unchanged | | `KV_TXN_MAYBE_COMMITTED` and any other error | TSO given up | Unchanged | | `OK`, `TXN_ALREADY_VISIBLE`, `TXN_ALREADY_ABORTED` | Transaction finished | Unchanged | ### Release note Fix a bounded incremental read in cloud mode that could miss a transaction whose commit in MetaService timed out. ### Check List (For Author) - Test: Unit Test - `CloudCommittedTsoTest#testKvCommitErrorIsFencedButDefiniteFailureIsReleased`: `KV_TXN_COMMIT_ERR` fences MetaService before the TSO is given up and still fails the commit; a fence that fails fails the commit with the fence error and gives nothing up; `KV_TXN_TOO_OLD` still gives the TSO up without a fence. It fails without the fix. The other three tests of the class pass. - Behavior changed: Yes. A commit that MetaService answers with `KV_TXN_COMMIT_ERR` costs one more MetaService RPC, the fence, and the commits in flight at or below the fenced TSO are rejected once and retried with a new TSO, as after a failed RPC. If the fence fails, the TSO service deactivates until it is calibrated again. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The change fixes the data-correctness hazard for genuinely ambiguous MetaService commit outcomes: CloudGlobalTransactionMgr now fences MetaService before releasing the commit TSO, and fence failures propagate through the existing FE cleanup path. The new unit test covers the uncertain response, fence failure, and definite KV_TXN_TOO_OLD release.
One P2 availability issue remains inline. MetaService also collapses exhausted KV_TXN_STORE_COMMIT_RETRYABLE responses originating from TXN_RETRYABLE_NOT_COMMITTED into KV_TXN_COMMIT_ERR. The FE branch therefore fences a known-not-committed attempt, unnecessarily invalidating lower in-flight TSOs and risking retry fan-out or TSO-service deactivation when the extra fence RPC fails.
Review checkpoints: the cloud transaction lifecycle and TSO tracker ordering were traced through commitTxn, MetaService retry/status normalization, advanceTsoFence, and TSOService; fencing precedes exact tracker removal and failure deactivates the service safely. No lock-order, callback, persistence, FE-BE protocol, or non-cloud parallel-path regression was found in this two-file patch. Error propagation is checked at the fence boundary. The added test setup and status-code normalization were independently rechecked. No user-provided focus points or existing inline blocking findings were present. Review was static only; no builds or tests were executed.
Review converged after 2 round(s).
| } else if (code == MetaServiceCode.OK || code == MetaServiceCode.TXN_ALREADY_VISIBLE | ||
| || code == MetaServiceCode.TXN_ALREADY_ABORTED) { | ||
| Env.getCurrentEnv().getTSOService().markTxnFinished(commitTxnRequest.getDbId(), transactionId); | ||
| } else if (code == MetaServiceCode.KV_TXN_COMMIT_ERR) { |
Contributor
There was a problem hiding this comment.
[P2] Avoid fencing known-not-committed retry exhaustion
MetaService::call_impl also rewrites an exhausted KV_TXN_STORE_COMMIT_RETRYABLE—which cast_as<COMMIT> produces for TxnErrorCode::TXN_RETRYABLE_NOT_COMMITTED—to KV_TXN_COMMIT_ERR. That source status explicitly means the FDB transaction did not commit, so this TSO can be abandoned directly. Because this branch cannot distinguish it from a timeout or unknown commit, it advances the global fence anyway, rejecting every in-flight TSO at or below this one with TXN_COMMIT_TSO_EXPIRED and potentially deactivating the whole TSO service if the extra fence RPC fails. Preserve the underlying status or fence only genuinely ambiguous commit results.
Contributor
TPC-H: Total hot run time: 28399 ms |
Contributor
TPC-DS: Total hot run time: 151058 ms |
Contributor
ClickBench: Total hot run time: 24.37 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
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
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: None
Related PR: #67820, #69831
Problem Summary:
In short: in cloud mode, when MetaService answers a commit with
KV_TXN_COMMIT_ERR, the FE gives up the commit TSO at once, as for a commit that failed for good. MetaService gives that answer when the FDB commit of the transaction timed out, and such a commit may still be applied afterwards. The committed TSO watermark can then pass a commit that is yet to land, and a bounded@incrread admitted at that watermark misses its rows. This PR advances the MetaService TSO fence to the TSO before giving it up on this answer, as the FE already does when the commit RPC itself fails.Background
commit_txnto MetaService ([fix](binlog) Track committed TSO and fence uncertain commits for bounded incremental reads #67820). The committed TSO watermark stays below the oldest registered TSO, and a bounded@incrread whose window ends at or below the watermark is admitted without waiting (TSOService.waitForReadableWindow), on the guarantee that no commit with a TSO at or below it can still land.TSOService.fenceAndAbandonCommitTso):commit_txnreads the fence in the FDB transaction it commits and rejects a TSO at or below it, so once the fence is advanced, the commit has landed or can no longer land. Every other answer gives the TSO up at once.fdb_txn_timeout_ms, 10 seconds by default) and turns the error of an FDB commit into its answer throughcast_as<ErrCategory::COMMIT>: a conflict isKV_TXN_CONFLICT,commit_unknown_resultisKV_TXN_MAYBE_COMMITTED, and a timeout (FDB errors 1004timed_outand 1031transaction_timed_out) or an error it does not classify isKV_TXN_COMMIT_ERR.The problem, and what it cost
KV_TXN_COMMIT_ERRdoes not settle the outcome of the commit.CloudGlobalTransactionMgr.commitTxngives the TSO up on that answer without a fence. Once nothing older is registered, the watermark passes the TSO while the commit can still land.@incrread admitted at that watermark misses the rows of a commit that lands later, and a read of the next window does not see them either: their TSO lies in the window already read. Anything else that takes the watermark as proof that no commit at or below it is coming is exposed the same way; the keep-alive of IVM streams in [fix](ivm) Keep the offsets of idle IVM stream partitions alive #69831 fences MetaService itself before it reads, for this reason.How this PR fixes it
KV_TXN_COMMIT_ERRtakes the path of a failed RPC:fenceAndAbandonCommitTsoadvances the fence to the TSO and gives the TSO up only after that.Results
commit_txnKV_TXN_COMMIT_ERR(the FDB commit timed out, or failed with an error MetaService does not classify)KV_TXN_MAYBE_COMMITTED,KV_TXN_TOO_OLDand the other errorsOK,TXN_ALREADY_VISIBLE,TXN_ALREADY_ABORTEDThe cost stays on that error path: one more MetaService RPC, and the commits in flight with a TSO at or below the fence are rejected once with
TXN_COMMIT_TSO_EXPIREDand retried with a new TSO, as after a failed RPC.Classes, and how they call each other
CloudGlobalTransactionMgr.commitTxn: the new branch forKV_TXN_COMMIT_ERR.CloudCommittedTsoTest: the newtestKvCommitErrorIsFencedButDefiniteFailureIsReleased.Not changed: MetaService, which still answers a timed out commit with
KV_TXN_COMMIT_ERR, and the handling ofKV_TXN_MAYBE_COMMITTED.Release note
Fix a bounded incremental read (
@incr) in cloud mode that could miss a transaction whose commit timed out in MetaService.Check List (For Author)
Test
CloudCommittedTsoTest#testKvCommitErrorIsFencedButDefiniteFailureIsReleased:KV_TXN_COMMIT_ERRadvances the fence before the TSO is given up and still fails the commit; a fence that fails fails the commit with the fence error and gives nothing up;KV_TXN_TOO_OLDstill gives the TSO up without a fence. It fails on the code before this PR. The cloud transaction and TSO test classes (org/apache/doris/cloud/transaction/*Testandorg/apache/doris/tso/*Test, 7 classes, 133 tests) pass.Behavior changed:
A commit that MetaService answers with
KV_TXN_COMMIT_ERRcosts one more MetaService RPC, and the commits in flight at or below its TSO are retried once with a new TSO. If that fence fails, the TSO service deactivates until it is calibrated again, as after a failed commit RPC.Does this need documentation?
Check List (For Reviewer who merge this PR)