spanconfig: improve use of commit ts validity intervals#80196
spanconfig: improve use of commit ts validity intervals#80196craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
854fd5f to
26466fc
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @arulajmani and @irfansharif)
pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go, line 289 at r1 (raw file):
} if maxCommitTS.Less(txn.ReadTimestamp()) {
should this check ProvisionalCommitTimestamp instead of ReadTimestamp? I know that the below code does the check based on the ReadTimestamp. I guess I'm wondering whether that's correct.
pkg/spanconfig/errors.go, line 22 at r1 (raw file):
// CommitTimestampOutOfBoundsError is returned when it's not possible to commit // an update within the specified time interval. type CommitTimestampOutOfBoundsError struct{}
I'm 50/50 on exporting this. I don't know that it adds any value, but I don't feel strongly.
26466fc to
8882f75
Compare
irfansharif
left a comment
There was a problem hiding this comment.
Thanks!
bors r+
Reviewable status:
pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go, line 289 at r1 (raw file):
Previously, ajwerner wrote…
should this check
ProvisionalCommitTimestampinstead ofReadTimestamp? I know that the below code does the check based on theReadTimestamp. I guess I'm wondering whether that's correct.
Yea I was wondering the same. I also found #76727 earlier today which fires the assertion under txn.UpdateDeadline, but doesn't look directly relevant. Looks like the check below dates back to 6acbe92 and was pointing to the txn's (static?) original timestamp. I'm not sure what would break if we only allowed deadlines to be set if under the provisional timestamp.
Back to this PR: if we're comparing the maxCommitTS against ReadTimestamp here, and if that's different from the ProvisionalCommitTimestamp, we can have the following scenarios (given ReadTimestamp <= ProvisionalTimestamp <= FinalCommitTS):
a. maxCommitTS < ReadTimestamp implies maxCommitTS < ProvisionalTimestamp
b. maxCommitTS >= ReadTimestamp
i. maxCommitTS <= ProvisionalTimestamp
ii. maxCommitTS > ProvisionalTimestamp
For both (b.i) and (b.ii) we can set the deadline without error-ing and will retry if maxCommitTS > FinalCommitTS. I'll keep the patch as is for now.
pkg/spanconfig/errors.go, line 22 at r1 (raw file):
Previously, ajwerner wrote…
I'm 50/50 on exporting this. I don't know that it adds any value, but I don't feel strongly.
With NewCommitTimestampOutOfBoundsError, the linter'll complains if it's unexported; I also wanted to avoid the reconciler -> kvaccessor dependency. But most of all I wanted to refer to a public symbol in various comments (KVAccessor, the RPCs).
|
Build failed (retrying...): |
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/spanconfig/errors.go, line 22 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
With NewCommitTimestampOutOfBoundsError, the linter'll complains if it's unexported; I also wanted to avoid the reconciler -> kvaccessor dependency. But most of all I wanted to refer to a public symbol in various comments (KVAccessor, the RPCs).
I don't follow. I'd just have NewCommitTimestampOutOfBoundsError return error
8882f75 to
c8bb9a2
Compare
|
Canceled. |
irfansharif
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
pkg/spanconfig/errors.go, line 22 at r1 (raw file):
Previously, ajwerner wrote…
I don't follow. I'd just have
NewCommitTimestampOutOfBoundsErrorreturnerror
Doh, done.
|
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
- Fix a bug where the sqlliveness session was incorrectly returning hlc.MinTimestamp when asking for the start timestamp of the underlying lease; - Drop the "lease" verbiage from KVAccessor, instead talk about the valid commit timestamp interval. Not all uses of the KVAccessor plumb in lease timestamps; - Check whether the max commit timestamp is non-empty (possible if the lease has already expired) before issuing the RPC, saves an extra round trip. - Unify handling of commit timestamp interval -- we were previously checking properties only in codepaths where the txn was not already scoped; - Move the retryable commit-timestamp-out-of-bounds error out to the base spanconfig package (breaking the dependency between spanconfigreconciler and spanconfigkvaccessor); - Re-write a test to use manual clocks, and use it to test the scenario where a txn's timestamp is bumped past the maximum commit timestamp (can happen for large kvaccessor batches, we should observe an internal retry); - Remove an extraneous goroutine from a test checking whether ctx cancellation is respected. Release note: None Release justification: low risk, high benefit change
c8bb9a2 to
064a43f
Compare
|
Bouncing the CLA bot. bors r+ |
|
Stopped waiting for PR status (Github check) without running due to duplicate requests to run. You may check Bors to see that this PR is included in a batch by one of the other requests. |
|
Build succeeded: |
hlc.MinTimestamp when asking for the start timestamp of the underlying
lease;
valid commit timestamp interval. Not all uses of the KVAccessor plumb
in lease timestamps;
lease has already expired) before issuing the RPC, saves an extra
round trip;
checking properties only in codepaths where the txn was not already
scoped;
base spanconfig package (breaking the dependency between
spanconfigreconciler and spanconfigkvaccessor);
where a txn's timestamp is bumped past the maximum commit timestamp
(can happen for large kvaccessor batches, we should observe an
internal retry);
cancellation is respected.
(re-) Fixes #73789.
Release note: None
Release justification: low risk, high benefit change