Skip to content

kv: remove TxnCoordSender.PrepareRetryableError, rationalize ManualRestart#105109

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/fixGenForcedErr
Jul 25, 2023
Merged

kv: remove TxnCoordSender.PrepareRetryableError, rationalize ManualRestart#105109
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/fixGenForcedErr

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Jun 17, 2023

This commit cleans up the implementation of GenerateForcedRetryableError by removing the TxnCoordSender.PrepareRetryableError method and having the TxnCoordSender.ManualRestart return a retryable error.

The cleanup also ensures that the TxnCoordSender is left in a txnRetryableError state after a ManualRestart, so that Txn.PrepareForRetry must be called before continuing to use the transaction.

The goal with both of these changes is to close the gap between the handling of error-driven txn restarts and manual restarts. It also reworks the code to construct the retry error in the same place that "handles" the error, which is important for future changes that plan to add more context to retry errors.

Release note: None
Epic: None

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

…start

This commit cleans up the implementation of `GenerateForcedRetryableError` by
removing the `TxnCoordSender.PrepareRetryableError` method and having the
`TxnCoordSender.ManualRestart` return a retryable error.

The cleanup also ensures that the `TxnCoordSender` is left in a
`txnRetryableError` state after a `ManualRestart`, so that `Txn.PrepareForRetry`
must be called before continuing to use the transaction.

The goal with both of these changes is to close the gap between the handling of
error-driven txn restarts and manual restarts. It also reworks the code to
construct the retry error in the same place that "handles" the error, which is
important for future changes that plan to add more context to retry errors.

Release note: None
Epic: None
@nvb nvb force-pushed the nvanbenschoten/fixGenForcedErr branch from 8b8a1be to fecca8d Compare July 24, 2023 18:26
@nvb nvb requested review from arulajmani and lidorcarmel July 24, 2023 18:26
@nvb nvb marked this pull request as ready for review July 24, 2023 18:26
@nvb nvb requested a review from a team as a code owner July 24, 2023 18:26
@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Jul 24, 2023

This is preparatory cleanup for #105161, which is necessary for Read Committed.

@arulajmani I assigned you as the primary reviewer.

@lidorcarmel I assigned you as a secondary reviewer, given your prior work in this area which cleanup up so much of this and made #105161 remotely possible.

Copy link
Copy Markdown
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @lidorcarmel)

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Jul 25, 2023

TFTR!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 25, 2023

Timed out.

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Jul 25, 2023

bors r=arulajmani

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 25, 2023

Build succeeded:

@craig craig bot merged commit 8551d57 into cockroachdb:master Jul 25, 2023
@nvb nvb deleted the nvanbenschoten/fixGenForcedErr branch July 25, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants