Record commit stats on the automatic idempotency recovery path#13388
Open
PierreZ wants to merge 1 commit into
Open
Record commit stats on the automatic idempotency recovery path#13388PierreZ wants to merge 1 commit into
PierreZ wants to merge 1 commit into
Conversation
When a transaction commits via the automatic idempotency recovery path (after commit_unknown_result, once determineCommitStatus discovers the commit actually succeeded), tryCommit set committedVersion but skipped the rest of the post-commit bookkeeping the normal success path performs. As a result, idempotency-recovered commits were not counted in transactionsCommitCompleted (undercounting the commit-throughput view), the committed-mutation stats were undercounted, the cached read version was not refreshed, and numErrors was not reset. Extract the shared bookkeeping into recordSuccessfulCommit() and call it from both the normal path and the recovery path: updateCachedReadVersion, committedVersion, numErrors reset, transactionsCommitCompleted, and the committed-mutation counters. The metadata-version cache update stays inline in the normal path: it needs the proxy reply's metadataVersion, which the recovery path's CommitResult does not carry. The commit-latency histograms and the EventCommit_V2 client log are also left to the normal path, since the recovery path's elapsed time includes failover and determineCommitStatus and would skew those metrics. Follow-up to apple#13038. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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
1 participant
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.
Follow-up to #13038, as promised there ("Will backport it to 7.4, and do a follow-up for main"). Part of the ongoing idempotency-id stabilization work (#12344).
Background
#13038 (closes #12582) fixed a client-side gap in
tryCommit: on the automatic idempotency replay path,determineCommitStatus()recovers the original commit version aftercommit_unknown_result, but the result was never written intotrState->committedVersion, sogetCommittedVersion()returnedinvalidVersionafter a successful replay. That was a one-line fix mirroring the normal-path assignment.While reviewing #13038 it was noted (I raised it on the PR: "did you think I should address the other gaps in the recovery path?") that the replay success block still skips the rest of the post-commit bookkeeping the normal path performs. This PR closes those remaining gaps.
Change
Extract the shared bookkeeping into a
recordSuccessfulCommit()helper, called from both the normal success path and the replay path, so idempotency-recovered commits now also:transactionsCommitCompleted(previously uncounted, which undercounts the commit-throughput view, e.g. theMovingDatatrace event),transactionCommittedMutations/transactionCommittedMutationBytes,updateCachedReadVersion),numErrors.Intentionally left to the normal path:
CommitResultfromdetermineCommitStatus()carries onlycommitVersionandbatchIndex, nometadataVersion, so there is nothing to cache there;commitLatencies/latencieshistograms and theEventCommit_V2client log: the replay path's elapsed time includes failover detection +commitDummyTransaction+ thedetermineCommitStatusread loop, which would skew the proxy-commit-latency view.No wire / system keyspace changes.
Test plan
tests/fast/AutomaticIdempotency.tomlsimulation (buggify on) across 10 seeds: all pass, replay path exercised 30 times total (DetermineCommitStatus Committed=1/AutomaticIdempotencyCommittedCODE_PROBE), noIdempotencyCommitMissingVersion.🤖 Generated with Claude Code