storage,util/hlc: convert hlc.Timestamp to proto3 #18910
Merged
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 |
petermattis
force-pushed
the
pmattis/hlc-timestamp-encoding
branch
from
19f5f91
to
22d509b
Compare
Convert hlc.Timestamp to proto3 so that a logical component of 0 will not be encoded on the wire. This is complicated by needing to leave the "below raft" encoding for MVCCMetadata unchanged. Added LegacyTimestamp to accomplish this which is convertible to Timestamp but has the legacy encoding. Changed the MVCCMetadata timestamp fields to use LegacyTimestamp. Note that the timestamp in MVCCMetadata.Txn (type TxnMeta) does not need to change because we never encoding an MVCCMetadata with a non-nil Txn field below raft. Extended the TrackRaftProtos mechanism to verify that TxnMeta is never encoded blow raft.
petermattis
force-pushed
the
pmattis/hlc-timestamp-encoding
branch
from
22d509b
to
1eead48
Compare
|
Reviewed 21 of 21 files at r1. pkg/storage/track_raft_protos.go, line 62 at r1 (raw file):
I don't understand why this is needed. Shouldn't this be traversed automatically? Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful. pkg/storage/track_raft_protos.go, line 62 at r1 (raw file): Previously, bdarnell (Ben Darnell) wrote…
That's what I thought initially, but that's not the way Comments from Reviewable |
petermattis
added a commit
to petermattis/cockroach
that referenced
this pull request
Oct 2, 2017
The encoding of `storage.Liveness` changed in cockroachdb#18910 which caused node liveness updates to fail in mixed version clusters due to failed conditional puts. Because the encoding changed, continued attempts to update the liveness would never succeed. Changed `Liveness.expiration` to use the `LegacyTimestamp` type. Audited the code and the only protos that are used as the expected value in conditional put operations are `storage.Liveness` and `roachpb.RangeDescriptor`. Extended TestBelowRaftProtos to verify the encoding of these protos does not change unexpectedly. Consistency checks were failing in mixed version clusters because of an explicit call to marshal an `hlc.Timestamp` in `Replica.sha512`. Fixes cockroachdb#18940
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.
Convert hlc.Timestamp to proto3 so that a logical component of 0 will
not be encoded on the wire. This is complicated by needing to leave the
"below raft" encoding for MVCCMetadata unchanged. Added LegacyTimestamp
to accomplish this which is convertible to Timestamp but has the legacy
encoding. Changed the MVCCMetadata timestamp fields to use
LegacyTimestamp. Note that the timestamp in MVCCMetadata.Txn (type
TxnMeta) does not need to change because we never encoding an
MVCCMetadata with a non-nil Txn field below raft. Extended the
TrackRaftProtos mechanism to verify that TxnMeta is never encoded blow
raft.