kv: pass explicit Now timestamp on BatchRequest, remove timestamp downcasting#85764
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Aug 12, 2022
Merged
Conversation
Member
|
This change is |
9f86bd6 to
0b4cc7d
Compare
tbg
approved these changes
Aug 10, 2022
Member
tbg
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained
Contributor
Author
|
TFTR! Was this implicitly an approval for #76233 as well, or does that still need another look? |
tbg
approved these changes
Aug 11, 2022
| // succeeded. | ||
| // TODO(nvanbenschoten): remove this in v23.1 when we remove the synthetic | ||
| // timestamp bit. | ||
| func (t Timestamp) DeprecatedTryToClockTimestamp() (ClockTimestamp, bool) { |
0b4cc7d to
90e191d
Compare
Contributor
Author
|
TFTR! bors r+ |
Contributor
|
Build failed (retrying...): |
Contributor
|
Build failed (retrying...): |
Contributor
Author
|
bors r- This seems to be causing issues in bors. CI was all green until the last rebase, so something might be up. |
Contributor
|
Canceled. |
Contributor
Author
|
I think it might have skewed with #83213. The benchmark ( |
…ncasting
This commit adds an explicit `ClockTimestamp` field called `Now` to the
`BatchRequest` header, which mirrors the `Now` field on the `BatchResponse`
header.
In doing so, it removes the last instance where we downcasted a `Timestamp` to a
`ClockTimestamp` using the `TryToClockTimestamp` method. With this change, MVCC
("operation") timestamps never flow back into HLC clocks as clock signals. This
was enabled by cockroachdb#80706 and sets the groundwork to remove synthetic timestamps in
v23.1 — the role they played in dynamic typing of clock timestamps is now
entirely fulfilled by statically typed `ClockTimestamp` channels.
This is an important step in separating out the MVCC timestamp domain from the
clock timestamp domain and clarifying the roles of the two layers. In turn, this
layering opens the door for CockroachDB to start thinking about dynamic clock
synchronization error bounds.
90e191d to
c7c154a
Compare
Contributor
Author
|
bors r+ |
Contributor
|
Build failed (retrying...): |
Contributor
|
Build succeeded: |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This commit adds an explicit
ClockTimestampfield calledNowto theBatchRequestheader, which mirrors theNowfield on theBatchResponseheader.In doing so, it removes the last instance where we downcasted a
Timestampto aClockTimestampusing theTryToClockTimestampmethod. With this change, MVCC ("operation") timestamps never flow back into HLC clocks as clock signals. This was enabled by #80706 and sets the groundwork to remove synthetic timestamps in v23.1 — the role they played in dynamic typing of clock timestamps is now entirely fulfilled by statically typed ClockTimestamp channels.This is an important step in separating out the MVCC timestamp domain from the clock timestamp domain and clarifying the roles of the two layers. In turn, this layering opens the door for CockroachDB to start thinking about dynamic clock synchronization error bounds.