kvcoord: simplify txn already committed assertion#73843
kvcoord: simplify txn already committed assertion#73843craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
tbg
commented
Dec 15, 2021
- kvcoord: improve TestCommitSanityCheckAssertionFiresOnUndetectedAmbiguousCommit
- kvcoord: simplify txn already committed assertion
|
This change is |
| if errTxn := pErr.GetTxn(); errTxn != nil { | ||
| if errTxn.Status == roachpb.COMMITTED { | ||
| pErr = sanityCheckCommittedErr(ctx, pErr, ba) | ||
| if err := maybeSanityCheckCommittedErr(ctx, pErr, ba, &tc.testingKnobs); err != nil { |
There was a problem hiding this comment.
nit: I think previous version was more readable because of explicit check for COMMITTED status prior to trying a sanity check. With new version I had to drill down into impl to understand why we feed all errors into commit check.
There was a problem hiding this comment.
interesting, the reason I pulled it in is because I had the opposite problem, that in the assertion we had to trust the caller that they had already checked the status. I felt that it needed to be either all in the method or completely inline, and I didn't want to inline it to keep the main code.
Were you thrown off by the name of the method referencing "committed"? We can rename it so that it's a general sanity check that.
There was a problem hiding this comment.
I think a better name would make sense. But I failed to come up with something sensible myself.
nvb
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @tbg)
pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 919 at r2 (raw file):
Previously, aliher1911 (Oleg) wrote…
I think a better name would make sense. But I failed to come up with something sensible myself.
I had the same reaction as Oleg. It's strange to see pErr passed to a function called maybeSanityCheckCommittedErr that accepts a param called pErrWithCommittedTxn before we've determined that the pErr has a committed txn.
If we're going to restructure the code like this, the function should be called maybeSanityCheckErr, its comment should be made more general, and the parameter should be called pErr. Or go back to what we had before.
pkg/kv/kvclient/kvcoord/replayed_commit_test.go, line 105 at r1 (raw file):
return err } // We need to give the txn an external lock (i.e. one on a different range),
Do you want to disable async intent resolution so that this is not racy? There's an intent resolver testing knob for that.
pkg/kv/kvclient/kvcoord/replayed_commit_test.go, line 79 at r2 (raw file):
}, }, nil },
nit: The indentation looks off here. At first I didn't even think it would compile or pass gofmt, but I see why it does. Still, I think it's worth fixing.
…uousCommit It was overriding `txnAutoGC`, which is not a clean knob to use, and it was stating an incorrect reason for even using that knob. Now we're preventing GC of the txn record by writing an additional intent, which is cleaner. Release note: None
79745c0 to
4fcfa00
Compare
tbg
left a comment
There was a problem hiding this comment.
TFTRs, PTAL
Reviewable status:
pkg/kv/kvclient/kvcoord/replayed_commit_test.go, line 105 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Do you want to disable async intent resolution so that this is not racy? There's an intent resolver testing knob for that.
Good idea, done.
nvb
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status:
Also improves TestCommitSanityCheckAssertionFiresOnUndetectedAmbiguousCommit to use a testing knob as opposed to updating the env var, which is always going to carry some residual risk of a data race. Release note: None
4fcfa00 to
1de1865
Compare
|
TFTR! bors r=nvanbenschoten |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |