morningman opened a new pull request, #69925:
URL: https://github.com/apache/doris/pull/69925
### 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 `@incr` read 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
- In cloud mode the Master FE gives each commit a TSO and registers it
before sending `commit_txn` to MetaService (#67820). The committed TSO
watermark stays below the oldest registered TSO, and a bounded `@incr` read
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.
- So a TSO is given up only once the outcome of its commit is settled. An
answer of success, already visible or already aborted finishes the transaction.
A failed RPC (a timeout, a lost connection) leaves the outcome open, so the FE
first advances the MetaService TSO fence to the TSO
(`TSOService.fenceAndAbandonCommitTso`): `commit_txn` reads 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.
- MetaService gives each FDB transaction a timeout (`fdb_txn_timeout_ms`, 10
seconds by default) and turns the error of an FDB commit into its answer
through `cast_as<ErrCategory::COMMIT>`: a conflict is `KV_TXN_CONFLICT`,
`commit_unknown_result` is `KV_TXN_MAYBE_COMMITTED`, and a timeout (FDB errors
1004 `timed_out` and 1031 `transaction_timed_out`) or an error it does not
classify is `KV_TXN_COMMIT_ERR`.
#### The problem, and what it cost
- A timeout cancels the FDB transaction, and the FoundationDB C API
documents that for a transaction cancelled after its commit was sent, "the
commit may have occurred or may occur in the future". So `KV_TXN_COMMIT_ERR`
does not settle the outcome of the commit.
- `CloudGlobalTransactionMgr.commitTxn` gives 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.
- A bounded `@incr` read 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 #69831 fences MetaService itself before
it reads, for this reason.
#### How this PR fixes it
- `KV_TXN_COMMIT_ERR` takes the path of a failed RPC:
`fenceAndAbandonCommitTso` advances the fence to the TSO and gives the TSO up
only after that.
- A fence that fails deactivates the TSO service, as after a failed RPC: no
TSO is given out and no read is admitted on the watermark until a calibration
has advanced the fence above every TSO given out before. The commit fails with
the fence error then, and the answer of MetaService is logged first, so it is
not lost.
- The commit fails as before in every case: only what happens to its TSO
changes.
#### Results
| Answer of MetaService to `commit_txn` | Before this PR | After this PR |
| --- | --- | --- |
| `KV_TXN_COMMIT_ERR` (the FDB commit timed out, or failed with an error
MetaService does not classify) | TSO given up at once | Fence advanced to the
TSO, then the TSO given up |
| The RPC failed | Fence advanced to the TSO, then the TSO given up |
Unchanged |
| `KV_TXN_MAYBE_COMMITTED`, `KV_TXN_TOO_OLD` and the other errors | TSO
given up at once | Unchanged |
| `OK`, `TXN_ALREADY_VISIBLE`, `TXN_ALREADY_ABORTED` | Transaction finished
| Unchanged |
The 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_EXPIRED` and retried with a new TSO, as after a failed RPC.
#### Classes, and how they call each other
- `CloudGlobalTransactionMgr.commitTxn`: the new branch for
`KV_TXN_COMMIT_ERR`.
- `CloudCommittedTsoTest`: the new
`testKvCommitErrorIsFencedButDefiniteFailureIsReleased`.
```
CloudGlobalTransactionMgr.commitTxn
|-- MetaServiceProxy.commitTxn -> answer, or an exception
|-- RPC failed, or KV_TXN_COMMIT_ERR (new) ->
TSOService.fenceAndAbandonCommitTso
| |-- GlobalTransactionMgr.advanceTsoFence(tso) then
TSOTransactionTracker.abandonCommitTso
| `-- the fence failed: TSOService.deactivate, and the commit fails
with the fence error
|-- KV_TXN_MAYBE_COMMITTED, other errors ->
TSOService.abandonCommitTso
`-- OK, TXN_ALREADY_VISIBLE, TXN_ALREADY_ABORTED ->
TSOService.markTxnFinished
```
Not changed: MetaService, which still answers a timed out commit with
`KV_TXN_COMMIT_ERR`, and the handling of `KV_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 <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
`CloudCommittedTsoTest#testKvCommitErrorIsFencedButDefiniteFailureIsReleased`:
`KV_TXN_COMMIT_ERR` advances 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_OLD` still 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/*Test` and `org/apache/doris/tso/*Test`, 7
classes, 133 tests) pass.
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
A commit that MetaService answers with `KV_TXN_COMMIT_ERR` costs 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?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
