kvclient, roachpb: cleanup the CPut API#50408
Conversation
|
This change is |
tbg
left a comment
There was a problem hiding this comment.
Basically looks good, had a few comments, thanks for working this out.
Reviewed 9 of 9 files at r1, 27 of 27 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, @irfansharif, and @nvanbenschoten)
Makefile, line 1230 at r2 (raw file):
PROTO_MAPPINGS := $(PROTO_MAPPINGS)Mgoogle/protobuf/timestamp.proto=github.com/gogo/protobuf/types, PROTO_MAPPINGS := $(PROTO_MAPPINGS)Mgoogle/protobuf/any.proto=github.com/gogo/protobuf/types, PROTO_MAPPINGS := $(PROTO_MAPPINGS)Mgoogle/protobuf/wrappers.proto=github.com/gogo/protobuf/types,
?
pkg/roachpb/api.go, line 981 at r2 (raw file):
var expBytes *types.BytesValue if expValue != nil { expBytes = &types.BytesValue{
Why? This is an extra alloc, what value does this provide? Distinguishing between "nothing" and empty? You could also do that with an additional bool. (I'll defer to @nvanbenschoten on whether this is important enough to do, but it's the pattern I wished protobufs in Go used generally via a bitmap).
pkg/roachpb/api.proto, line 193 at r2 (raw file):
Value deprecated_exp_value = 3; // exp_bytes represents the expected existing value for the key. If missing, // the key is expected to not exist. If set to []byte{}, the value is expected
You can't set this to []byte{}, make this precise.
pkg/roachpb/api.proto, line 202 at r2 (raw file):
// // Note that the existing value's timestamp doesn't matter, only its data. So, // the CPut will succeed in ABA situations.
ABA?
andreimatei
left a comment
There was a problem hiding this comment.
prepended a commit. PTAL
Reviewable status:
Makefile, line 1230 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
?
for google.protobuf.BytesValue. But it's gone.
pkg/roachpb/api.go, line 981 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Why? This is an extra alloc, what value does this provide? Distinguishing between "nothing" and empty? You could also do that with an additional bool. (I'll defer to @nvanbenschoten on whether this is important enough to do, but it's the pattern I wished protobufs in Go used generally via a bitmap).
The distinction has gone away. See the first commit.
pkg/roachpb/api.proto, line 193 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
You can't set this to
[]byte{}, make this precise.
Well the intention was that you could set it to that. But I've done some digging and decided to remove the distinction between missing keys and keys with empty values, since the latter don't actually exist. See now.
pkg/roachpb/api.proto, line 202 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
ABA?
I thought it's a known term. Explained.
irfansharif
left a comment
There was a problem hiding this comment.
Reviewed 33 of 33 files at r3, 9 of 9 files at r4, 26 of 26 files at r5.
Reviewable status:
pkg/kv/kvserver/replica_command.go, line 1963 at r5 (raw file):
// parsed RangeDescriptor directly as the expected value in a CPut, but proto // message encodings aren't stable so this was fragile. Calling this method and // then passing the returned nytes as the expected value in a CPut does the same
s/nytes/bytes
pkg/roachpb/api.proto, line 182 at r5 (raw file):
// deprecated_exp_val represents the expected existing value for the key. If // the existing value is different, the request will return a // ConditionFailedError. A missing (Go nil) exp_value.raw_bytes means that the
s/exp_value/deprecated_exp_value
tbg
left a comment
There was a problem hiding this comment.
Looks good, I only reviewed the "clarify that Put() with nil value is not a thing" commit. Let me know if I should also re-visit anything else.
While thinking about the CPut API, the question about whether or not keys with empty values can exist. Comments on ConditionalPutRequest suggest that there is such a thing as an "empty value". A completely empty value represents a an MVCC deletion tombstone, so that part is clear, but more intriguing is the case of a key with a value that consists just of a checksum and nothing else. Allowing such values would arguably make sense, but it turns out that such KVs don't exist, since the Value would be invalid according to the Value.Verify() method which requires a full header (a checksum and a type tag). Correspondingly, our APIs don't provide ways to create a Value without some data in it (which data could be an empty byte array, for example, but that has a non-empty encoding). Figuring this out with any confidence was not trivial, though. This patch adds assertions to Batch.Put() against trying to use such empty values - serving mostly as documentation, and it also sprinkles some comments about the meaning of nil values to other APIs. Release note: None
In all other contexts, a Value's "data" does not include its tag (e.g. the Value.dataBytes() method), except for this method. The new name makes it explicit. Release note: None
andreimatei
left a comment
There was a problem hiding this comment.
TFTRs
bors r+
Reviewable status:
pkg/kv/kvserver/replica_command.go, line 1963 at r5 (raw file):
Previously, irfansharif (irfan sharif) wrote…
s/nytes/bytes
Done.
pkg/roachpb/api.proto, line 182 at r5 (raw file):
Previously, irfansharif (irfan sharif) wrote…
s/exp_value/deprecated_exp_value
done
Build failed |
The CPut API was funky: it took an expected value as a roachpb.Value, but the checksum in it and the timestamp didn't matter. This was undocumented. The checksum in particular covers both the key and the value, so its relationship with a CPut is weird (since the key for which we're using the value might be different from the key from whence it came). This choice of Value had consequences for the Batch API: Batch.CPut() was taking a *roachpb.Value, for which it cleared and then re-initialized the checksum. This was all surprising, and meant that some callers had to copy the underlying bytes for the value before passing them to CPut(). This patch changes all the interfaces to just take []byte. There's no longer a checksum or a timestamp to speak of. CPut() now also internally copies the expected value's bytes, so the caller no longer needs to worry about ownership. Release note: None
andreimatei
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
Canceled (will resume) |
Build succeeded |
The CPut API was funky: it took an expected value as a roachpb.Value,
but the checksum in it and the timestamp didn't matter. This was
undocumented. The checksum in particular covers both the key and the
value, so its relationship with a CPut is weird (since the key for
which we're using the value might be different from the key from whence
it came). This choice of Value had consequences for the Batch API:
Batch.CPut() was taking a *roachpb.Value, for which it cleared and then
re-initialized the checksum. This was all surprising, and meant that
some callers had to copy the underlying bytes for the value before
passing them to CPut().
This patch changes all the interfaces to just take []byte. There's no
longer a checksum or a timestamp to speak of. CPut() now also internally
copies the expected value's bytes, so the caller no longer needs to
worry about ownership.
Release note: None