kv: don't pass clock information through Raft log#76095
kv: don't pass clock information through Raft log#76095craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
d016065 to
e4aa771
Compare
tbg
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvserver/replica_proposal.go, line 864 at r2 (raw file):
res.Replicated.WriteTimestamp = ba.WriteTimestamp() } else { if !r.ClusterSettings().Version.IsActive(ctx, clusterversion.DontProposeWriteTimestampForLeaseTransfers) {
IsActive is moderately expensive (has to unmarshal a protobuf). I assume this code path is "cold enough" for this not to matter (since most requests will apply the timestamp cache) but wanted to point it out anyway.
pkg/kv/kvserver/kvserverpb/proposer_kv.proto, line 132 at r2 (raw file):
// of the command against the GC threshold and to update the followers' // clocks. Only set if the request that produced this command is a write that // cares about the timestamp cache.
This is deprecated now, right?
e4aa771 to
f16f5cd
Compare
This commit eliminates the primary mechanism that we use to pass clock information from a leaseholder, through Raft log entries, to a Range's followers. As we found in cockroachdb#72278, this was only needed for correctness in a few specific cases — namely lease transfers and range merges. These two operations continue to pass clock signals through more explicit channels, but we remove the unnecessary general case. The allows us to remote one of the two remaining places where we convert a `Timestamp` to a `ClockTimestamp` through the `TryToClockTimestamp` method. As outlined in cockroachdb#72121 (comment), I would like to remove ability to downcast a "data-plane" `Timestamp` to a "control-plane" `CloudTimestamp` entirely. This will clarify the role of `ClockTimestamps` in the system and clean up the channels through which clock information is passed between nodes. The other place where we cast from `Timestamp` to `ClockTimesatmp` is in `Store.Send`, at the boundary of KV RPCs. I would also like to get rid of this, but doing so needs to wait on cockroachdb#73732.
f16f5cd to
74e2070
Compare
nvb
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/kv/kvserver/replica_proposal.go, line 864 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
IsActiveis moderately expensive (has to unmarshal a protobuf). I assume this code path is "cold enough" for this not to matter (since most requests will apply the timestamp cache) but wanted to point it out anyway.
Yeah, this is cold enough that unmarshalling a proto seems ok. Has IsActive gotten more expensive recently though? I wasn't aware.
pkg/kv/kvserver/kvserverpb/proposer_kv.proto, line 132 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
This is deprecated now, right?
Not fully. It's still used for real MVCC writes that use it to check against the GC threshold (below Raft in checkForcedErr), to assert against closed timestamp violations (assertNoWriteBelowClosedTimestamp), and now for AddSSTable requests with the WriteAtRequestTimestamp flag set.
This first use seemed dubious to me and Andrei, but our attempts to understand it enough to get rid of it were unsuccessful, so I decided not to pick that fight in this PR. The HLC is a formidable enough foe 1-on-1.
|
Build succeeded: |
This commit eliminates the primary mechanism that we use to pass clock information from a leaseholder, through Raft log entries, to a Range's followers. As we found in #72278, this was only needed for correctness in a few specific cases — namely lease transfers and range merges. These two operations continue to pass clock signals through more explicit channels, but we remove the unnecessary general case.
The allows us to remote one of the two remaining places where we convert a
Timestampto aClockTimestampthrough theTryToClockTimestampmethod. As outlined in #72121 (comment), I would like to remove ability to downcast a "data-plane"Timestampto a "control-plane"CloudTimestampentirely. This will clarify the role ofClockTimestampsin the system and clean up the channels through which clock information is passed between nodes.The other place where we cast from
TimestamptoClockTimesatmpis inStore.Send, at the boundary of KV RPCs. I would also like to get rid of this, but doing so needs to wait on #73732.