Store value protobuf directly for MVCC values#49
Conversation
integer values for incrementable keys, we now marshal and unmarshal the Value protobuf directly. This allows us to Get incrementable keys and to scan them. It also allows us to correctly discern an error incrementing a key which is not a varint encoded value. Implemented value checksums and checksum verification, although only doing the verification now through kv/rest. Probably want to do the same elsewhere.
|
Also, @Tobstar87 and @bdarnell, please review! |
There was a problem hiding this comment.
do you wish to include the timestamp of Value into checksum?
There was a problem hiding this comment.
From many of the comments here, I'm thinking it might make sense to rename the Checksum field "EndToEndChecksum". The idea is this checksum is set by the client -- perhaps as far back as from a Javascript client speaking over http, and then verified by any similar endpoint. It's meant to capture corruption anywhere in the communication stack, up to and including the final network interface and even the client's own potential memory stomping bugs.
As such, there's no reliable way to include the timestamp--remember that timestamps on writes can change if part of a transactions, and for non-transactional writes, the timestamp is taken as the receiving node's wall clock time, so can't be incorporated by the original client.
There was a problem hiding this comment.
Computing the checksum in this way allows bytes to be shuffled between the key and value without changing the checksum. If we were using this checksum for security that would be a problem; I don't know if it matters here since we're using a weak checksum. If you want to do something about that I suggest hashing len(key) + key + len(value) + value.
There was a problem hiding this comment.
Hmm, not sure this addition will add much value to justify the complexity. This end-to-end checksum is mainly about verifying round-trip transport integrity from client to persistent db storage and then back to client.
|
it looks good to me in general. Two general comments:
|
storage/engine/mvcc.go
Outdated
|
LGTM |
Added proto.Response.Verify method to verify checksums and now do verification automatically on the responses of Get(), ConditionalPut() and Scan(). Also now always initialize checksums on Put() and ConditionalPut(). Eventually we're going to want to fully expose the checksums to non-go clients, but will have to evolve the KV API to include that capability.
Store value protobuf directly for MVCC values
Instead of storing byte strings manually and manually varint encoding
integer values for incrementable keys, we now marshal and unmarshal the
Value protobuf directly. This allows us to Get incrementable keys and
to scan them. It also allows us to correctly discern an error incrementing
a key which is not a varint encoded value.
Implemented value checksums and checksum verification, although only
doing the verification now through kv/rest. Probably want to do the
same elsewhere.