-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql: simplifiy tracking of injected txn retry errors #108817
Conversation
Release note: None
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @rafiss)
pkg/sql/txn_state.go
line 189 at r2 (raw file):
// Reset state vars to defaults. ts.sqlTimestamp = sqlTimestamp ts.isHistorical = false
Double checking that we don't need to reset injectedTxnRetryCounter
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @nvanbenschoten)
pkg/sql/txn_state.go
line 189 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Double checking that we don't need to reset
injectedTxnRetryCounter
here.
hm, well it could. the reason i did not is because i moved the counter into extraTxnState
. but after reading this more closely, i think it makes more sense to keep it in this txnState
instead. extraTxnState
is meant for things that need to be passed into the internal executor, which we don't actually want for this counter
Rather than using the txn epoch, we can just track how many errors were injected. This lets us have a bit more control over how many errors to inject, without having to rely on how the KV layer handles different types of transaction retries. Release note: None
3cf2e02
to
6ecbbc0
Compare
tftr! bors r+ |
This PR was included in a batch that was canceled, it will be automatically retried |
Build succeeded: |
Rather than using the txn epoch, we can just track how many errors were
injected. This lets us have a bit more control over how many errors to
inject, without having to rely on how the KV layer handles different
types of transaction retries.
informs: #100145
split off from: #107044
Release note: None