kvserver: use execution timestamps for verified when available #88550
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 |
tbg
force-pushed
the
kvnemesis-use-exec-ts
branch
from
22df830 to
0da53fb
Compare
tbg
force-pushed
the
kvnemesis-use-exec-ts
branch
from
0da53fb to
b02b03c
Compare
erikgrinaker
approved these changes
Sep 26, 2022
There was a problem hiding this comment.
We could ensure that AmbiguousResultError always contains the one possible commit timestamp. This should work since TxnCoordSender is always local to kvnemesis, and so there's no "fallible" component between the two.
+1. Will we have to extend AmbiguousResultError with a special case for this, or can we have some test-only side channel in TxnCoordSender that we can use to obtain this in a clearer way?
Now that "most" operations save their execution timestamps, use them for verification. This has the undesirable side effect of failing the entire test suite, which didn't bother specifying timestamps for most operations. Now they are required, and need to be present, at least for all mutations. I took the opportunity to also clean up the test helpers a bit, so now we don't have to pass an `error` when it's not required. The big remaining caveat is that units that return with an ambiguous result don't necessarily have a commit timestamp. I *think* this is only an implementation detail. We *could* ensure that `AmbiguousResultError` always contains the one possible commit timestamp. This should work since `TxnCoordSender` is always local to `kvnemesis`, and so there's no "fallible" component between the two. This would result in a significant simplification of `kvnemesis`, since as is when there are ambiguous deletions, we have to materialize them but cannot assign them a timestamp. This complicates various code paths and to be honest I'm not even sure what exactly we verify and how it all works when there are such "half-materialized" writes. I would rather do away with the concept altogether. Clearly we also won't be able to simplify the verification to simply use commit order if there are operations that don't have a timestamp, which is another reason to keep pushing on this. Release note: None
|
bors r=erikgrinaker |
tbg
force-pushed
the
kvnemesis-use-exec-ts
branch
from
b02b03c to
09043a3
Compare
|
Canceled. |
|
bors r=erikgrinaker |
|
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.
Now that "most" operations save their execution timestamps, use them
for verification.
This has the undesirable side effect of failing the entire test suite,
which didn't bother specifying timestamps for most operations.
Now they are required, and need to be present, at least for all
mutations.
I took the opportunity to also clean up the test helpers a bit,
so now we don't have to pass an
errorwhen it's not required.The big remaining caveat is that units that return with an ambiguous
result don't necessarily have a commit timestamp. I think this is only
an implementation detail. We could ensure that
AmbiguousResultErroralways contains the one possible commit timestamp. This should work
since
TxnCoordSenderis always local tokvnemesis, and so there'sno "fallible" component between the two.
This would result in a significant simplification of
kvnemesis, sinceas is when there are ambiguous deletions, we have to materialize them
but cannot assign them a timestamp. This complicates various code paths
and to be honest I'm not even sure what exactly we verify and how it all
works when there are such "half-materialized" writes. I would rather do
away with the concept altogether. Clearly we also won't be able to
simplify the verification to simply use commit order if there are
operations that don't have a timestamp, which is another reason to keep
pushing on this.
Release note: None