storage: avoid client-side retries on CPut/Inc write-too-old errors#22234
Conversation
|
This change is |
|
@nvanbenschoten, ignore first commit; that's the serializable txn retries. This is the change I was telling you about to avoid client-side transaction retries for conditional puts. @andreimatei this solves the case where the old auto-retry mechanism might have saved us from a client visible restart with conditional puts that we whiteboarded yesterday. |
|
The code all looks good. I do have a general question about Review status: 0 of 43 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
|
Because you end up with Review status: 0 of 43 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
de625fc to
3e00821
Compare
3e00821 to
6f0379f
Compare
6f0379f to
05a3800
Compare
|
OK, this is ready. I updated it to also include |
|
Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
|
yep Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
Previously, a conditional put op that experienced a write-too-old error would not immediately return an error. Instead, it would proceed to lay down the intent, set the `WriteTooOld` bool on the `Transaction`, and continue so that intents might be laid down to assure succes on the eventual retry. The `EndTransaction` request triggers restart at commit time in the event that `WriteTooOld` is set. The problem with this is that refreshing the conditional put key is guaranteed to fail because there is a known, newer version of the key written. This implies a client-side retry. This change makes conditional puts immediately return a `WriteTooOldError` instead of just swallowing the error and setting the `WriteTooOld` flag. The returned error allows the `TxnCoordSender` to refresh all spans mid-transaction, and hopefully retry the conditional put at the advanced timestamp, to either succeed or fail on the conditional put. This allows the transaction to continue on success, or return a `ConditionFailedError` on failure. Release note: None
05a3800 to
6e1ae41
Compare
|
Added fixes for a test in |
|
Spencer what was the test fix? Review status: 0 of 3 files reviewed at latest revision, all discussions resolved, all commit checks successful. Comments from Reviewable |
Previously, a conditional put or increment ops that experienced a write-too-old error
would not immediately return an error. Instead, it would proceed to lay
down the intent, set the
WriteTooOldbool on theTransaction, andcontinue so that intents might be laid down to assure succes on the
eventual retry. The
EndTransactionrequest triggers restart at committime in the event that
WriteTooOldis set.The problem with this is that refreshing the conditional put key is
guaranteed to fail because there is a known, newer version of the key
written. This implies a client-side retry.
This change makes conditional puts immediately return a
WriteTooOldErrorinstead of just swallowing the error and setting the
WriteTooOldflag. The returned error allows the
TxnCoordSenderto refresh allspans mid-transaction, and hopefully retry the conditional put at the
advanced timestamp, to either succeed or fail on the conditional put.
This allows the transaction to continue on success, or return a
ConditionFailedErroron failure.