kv: handle future-time operations for conflict resolution requests #59693
Conversation
|
This change is |
nvanbenschoten
force-pushed
the
nvanbenschoten/tscacheFutureReqs2
branch
2 times, most recently
from
516fc17
to
a9deb28
Compare
nvanbenschoten
force-pushed
the
nvanbenschoten/tscacheFutureReqs2
branch
from
a9deb28
to
423932f
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/roachpb/batch.go, line 28 at r2 (raw file):
//go:generate go run -tags gen-batch gen_batch.go // WriteTimestamp returns the timestamps at which this request is writing. For
Did you intent to add this to the Header, given that it's also defined below on the BatchRequest?
nvanbenschoten
force-pushed
the
nvanbenschoten/tscacheFutureReqs2
branch
from
423932f
to
59d5358
Compare
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status:
pkg/kv/kvserver/replica.go, line 1233 at r4 (raw file):
// If the request is a write or a consistent read, it requires the // replica serving it to hold the range lease. st, shouldExtend, err = r.leaseGoodToGoRLocked(ctx, now, ba.WriteTimestamp())
put a comment nodding to the fact that the max offset is accounted for inside the lease check
pkg/kv/kvserver/replica_tscache.go, line 57 at r5 (raw file):
// and is acquired by a new replica that begins serving writes immediately // to the same keys at the next lease's start time. if exp := st.Expiration(); exp.LessEq(ts) {
💯
pkg/kv/kvserver/batcheval/cmd_push_txn.go, line 119 at r4 (raw file):
// push the transaction to. Transactions should not be pushed into the // future past the leaseholder's lease expiration or their effect may // not be fully reflected in a future leaseholder's timestamp cache.
nit: make this comment more clear about the fact that we don't expect the client to send such a request (right?). Or actually make it an AssertionFailedError
pkg/kv/kvserver/batcheval/cmd_push_txn.go, line 123 at r4 (raw file):
h.Timestamp, args.PushTo) } if h.WriteTimestamp().Less(args.PusheeTxn.MinTimestamp) {
what's this condition about? This also feels like an assertion, right? Hitting it would mean that someone tried to push a txn
pkg/kv/kvserver/batcheval/cmd_query_intent.go, line 62 at r4 (raw file):
ownTxn = true } else { return result.Result{}, ErrTransactionUnsupported
consider making a note of this on QueryIntentRequest - that if the request is made inside a txn, it must be the intent's txn.
pkg/kv/kvserver/batcheval/cmd_query_intent.go, line 66 at r4 (raw file):
} if h.WriteTimestamp().Less(args.Txn.WriteTimestamp) { // This condition must hold for the timestamp cache update to be safe.
pls say a few more words here - hint that the ts cache update in question is the one done in case we fail to find the intent, and it's done @ args.Txn.WriteTimestamp
And this is also an assertion, isn't it?
pkg/kv/kvserver/batcheval/cmd_query_txn.go, line 54 at r4 (raw file):
} if h.WriteTimestamp().Less(args.Txn.MinTimestamp) { // This condition must hold for the timestamp cache access to be safe.
similar to above, consider clarifying this ts cache "access" more
Addresses a TODO.
nvanbenschoten
force-pushed
the
nvanbenschoten/tscacheFutureReqs2
branch
from
59d5358
to
b24e338
Compare
There was a problem hiding this comment.
TFTR!
I'll try to get this merged today to unblock #59566.
Reviewable status:
pkg/kv/kvserver/replica.go, line 1233 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
put a comment nodding to the fact that the max offset is accounted for inside the lease check
Done.
pkg/kv/kvserver/batcheval/cmd_push_txn.go, line 119 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
nit: make this comment more clear about the fact that we don't expect the client to send such a request (right?). Or actually make it an
AssertionFailedError
Done.
pkg/kv/kvserver/batcheval/cmd_push_txn.go, line 123 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
what's this condition about? This also feels like an assertion, right? Hitting it would mean that someone tried to push a txn
Done.
pkg/kv/kvserver/batcheval/cmd_query_intent.go, line 62 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
consider making a note of this on
QueryIntentRequest- that if the request is made inside a txn, it must be the intent's txn.
Done.
pkg/kv/kvserver/batcheval/cmd_query_intent.go, line 66 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
pls say a few more words here - hint that the ts cache update in question is the one done in case we fail to find the intent, and it's done @ args.Txn.WriteTimestamp
And this is also an assertion, isn't it?
Done.
pkg/kv/kvserver/batcheval/cmd_query_txn.go, line 54 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
similar to above, consider clarifying this ts cache "access" more
Done.
pkg/roachpb/batch.go, line 28 at r2 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Did you intent to add this to the Header, given that it's also defined below on the
BatchRequest?
Yes, because we don't have access to a BatchRequest in during request evaluation, just the BatchRequest.Header.
There was a problem hiding this comment.
Reviewable status:
pkg/roachpb/batch.go, line 28 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Yes, because we don't have access to a
BatchRequestin during request evaluation, just theBatchRequest.Header.
can you make the other one use this one?
Relates to cockroachdb#57688. This commit updates the following four requests types to properly handle future-time operations: - `PushTxnRequest` - `QueryTxnRequest` - `QueryIntentRequest` - `RecoverTxnRequest` It also updates the request evaluation code to properly check that all timestamp cache updates are safe, based on the batch header timestamps of the requests. The next commit will be adding a more strict assertion about proper timestamp cache use, so it's better that we catch these violations early. In doing so, the commit also updates the replica lease check to test against the batch's read or write timestamp, whichever is later. This addresses the concerns raised in cockroachdb#58904 (review).
This commit improves an existing (race-only) assertion to not only check timestamp cache updates against the current HLC clock, but to also do so against the lease that the request performing the update is evaluating under. This ensures that timestamp cache updates are not lost during non-cooperative lease change.
This commit replaces a number of calls to `errors.Errorf` with calls to `errors.AssertionFailedf` in request evaluation logic.
nvanbenschoten
force-pushed
the
nvanbenschoten/tscacheFutureReqs2
branch
from
b24e338
to
b15e3dd
Compare
There was a problem hiding this comment.
bors r+
Reviewable status:
pkg/roachpb/batch.go, line 28 at r2 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
can you make the other one use this one?
Done. I didn't realize I had pulled the other one in during a rebase. Header is embedded, so I just deleted the method on BatchRequest.
|
Build failed: |
|
Unrelated flake in bors r+ |
|
Build succeeded: |
Relates to #57688.
This PR updates the following four requests types to properly handle future-time operations:
PushTxnRequestQueryTxnRequestQueryIntentRequestRecoverTxnRequestIt also updates the request evaluation code to properly check that all timestamp cache updates are safe, based on the batch header timestamps of the requests. This is coupled with a stricter, more comprehensive assertion when updating the timestamp cache that all updates are performed below the expiration time of the active lease. This ensures that timestamp cache updates are not lost during non-cooperative lease change.
In doing so, the PR also updates the replica lease check to test against the batch's read or write timestamp, whichever is later. This addresses the concerns raised in #58904 (review).