morningman commented on code in PR #69925:
URL: https://github.com/apache/doris/pull/69925#discussion_r4442190817
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java:
##########
@@ -970,6 +970,19 @@ private TransactionState
commitTxn(CommitTxnRequest.Builder builder, List<Table>
} 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) {
Review Comment:
The mechanism is right: `MetaServiceProxy::call_impl` rewrites
`KV_TXN_STORE_COMMIT_RETRYABLE` to `KV_TXN_COMMIT_ERR` once the retries run
out, and the last attempt then did not commit, so that fence is not needed.
I am keeping it:
- A fence is always safe, and here it costs what it costs after a failed
RPC: one more MetaService RPC, and the commits in flight at or below the TSO
retry once with a new TSO. It takes five attempts over at least 7.5 seconds of
backoff, all failing with FDB throttling, `process_behind`, `database_locked`
or the like, so commits are failing anyway by then.
- The FE cannot tell this case apart. MetaService rewrites the answer from
the last attempt only, and the same loop also retries `KV_TXN_MAYBE_COMMITTED`,
so an earlier attempt may have had an unknown result. Fencing only the
ambiguous answers needs MetaService to report that across attempts, with a code
clients can see.
- Such a code changes the MetaService protocol: an older proto2 client may
read an enum value it does not know as unset and fall back to `OK`, which is
why #62244 keeps `KV_TXN_MAYBE_COMMITTED` inside MetaService. A new FE talking
to an older MetaService still gets `KV_TXN_COMMIT_ERR` and has to fence it.
That belongs to a MetaService change, not to this FE fix.
Checking this also showed that the description listed the sources of
`KV_TXN_COMMIT_ERR` too narrowly: since #62244 an unknown result reaches the FE
as `KV_TXN_COMMIT_ERR` as well, so this PR fences it too. I have updated the
description to list every source, including the one you point out, and the
trade-off above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
