storage,kv: don't return errors with the WriteTooOld flag set #45102
Conversation
This file contains 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
|
This change is |
nvanbenschoten
approved these changes
Feb 14, 2020
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 5 of 5 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)
pkg/kv/txn_interceptor_span_refresher.go, line 387 at r2 (raw file):
} func (sr *txnSpanRefresher) sendHelper(
What is the point of this?
PR cockroachdb#44507 has left an unnecessary check in place. The check tolerates requests with the WriteTooOld flag set for leaf txn, even though the same commit makes sure that leaves reset that flag. Release note: None
The span refresher interceptor is supposed to terminate the txn.WriteTooOld flag. For that, it asserts that the request doesn't not have that flag set on it; only responses should have it. There was, however, a case where terminating the flag was not happening as intended: when a non-retriable error was coming back with the WriteTooOld flag set, the flag made its way into the client's txn, slipping past the interceptor. This was causing further requests to fail the aforementioned assertion. (Since errors are at play here, the only further request should be a Rollback). This patch fixes this by having servers never return errors with this flag set. Setting this flag on an error does not make sense anyway. Touches cockroachdb#40786 Release note: None
Release note: None
andreimatei
force-pushed
the
txn.fix-write-too-old
branch
from
5c8b219 to
189151d
Compare
andreimatei
commented
Feb 14, 2020
There was a problem hiding this comment.
bors r+
Reviewable status:
pkg/kv/txn_interceptor_span_refresher.go, line 387 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
What is the point of this?
apparently none. Removed in the next commit.
craig bot
pushed a commit
that referenced
this pull request
Feb 14, 2020
45102: storage,kv: don't return errors with the WriteTooOld flag set r=andreimatei a=andreimatei The span refresher interceptor is supposed to terminate the txn.WriteTooOld flag. For that, it asserts that the request doesn't not have that flag set on it; only responses should have it. There was, however, a case where terminating the flag was not happening as intended: when a non-retriable error was coming back with the WriteTooOld flag set, the flag made its way into the client's txn, slipping past the interceptor. This was causing further requests to fail the aforementioned assertion. (Since errors are at play here, the only further request should be a Rollback). This patch fixes this by having servers never return errors with this flag set. Setting this flag on an error does not make sense anyway. Touches #40786 Release note: None Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The span refresher interceptor is supposed to terminate the
txn.WriteTooOld flag. For that, it asserts that the request doesn't not
have that flag set on it; only responses should have it. There was,
however, a case where terminating the flag was not happening as
intended: when a non-retriable error was coming back with the
WriteTooOld flag set, the flag made its way into the client's txn,
slipping past the interceptor. This was causing further requests to fail
the aforementioned assertion. (Since errors are at play here, the only
further request should be a Rollback).
This patch fixes this by having servers never return errors with this
flag set. Setting this flag on an error does not make sense anyway.
Touches #40786
Release note: None