Keys for the table data #1356
Keys for the table data #1356
Conversation
| // TableDataPrefix prefixes all Table data to aid in transitioning | ||
| // key:value data to Table data, and for ease of debugging. | ||
| TableDataPrefix = proto.Key("table-") | ||
| TableDataPrefixLength = 6 |
There was a problem hiding this comment.
s/6/len(TableDataPrefix)/g?
There was a problem hiding this comment.
Seeing how this value is used, I'd just nuke it and use len(TableDataPrefix) when needed.
|
@vivekmenezes Github is far inferior to Mondrian when it comes to code reviews. I do not receive notification when you've uploaded a new patch set and cannot easily tell when you've addressed a comment. It is good practice to respond to every comment someone makes on your PR with a "Done" or "Note done and here is why...". |
vivekmenezes
force-pushed
the
vivek/keys
branch
3 times, most recently
from
c77be71 to
38e4a05
Compare
|
Ready for another viewing |
| @@ -62,7 +59,7 @@ func NodeStatusKey(nodeID int32) proto.Key { | |||
|
|
|||
| // MakeNameMetadataKey returns the key for the namespace. | |||
| func MakeNameMetadataKey(parentID uint32, name string) proto.Key { | |||
| k := make([]byte, 0, len(NameMetadataPrefix)+maxVarintSize+len(name)) | |||
| k := make([]byte, 0, len(NameMetadataPrefix)+binary.MaxVarintLen64+len(name)) | |||
There was a problem hiding this comment.
We're not using encoding/binary for encoding our varints, so it is incorrect to use binary.MaxVarintLen64 here.
There was a problem hiding this comment.
I see it being used by Encoding() in
util/encoding/encoding.go
Should I rename maxVarintSize to MaxEncodeUvarint and export it from util/encoding/encoding.go package then?
There was a problem hiding this comment.
Should I rename maxVarintSize to MaxEncodeUvarint and export it from util/encoding/encoding.go package then?
Yep
There was a problem hiding this comment.
The use of binary.PutVarint in util/encoding.Encode should really be removed. Looks like the only use of that method is by engine.Increment() which appears to be unused.
There was a problem hiding this comment.
Will look at taking that out in a new PR
On Tue, Jun 16, 2015 at 10:14 AM Peter Mattis [email protected]
wrote:
In keys/keys.go
#1356 (comment):@@ -62,7 +59,7 @@ func NodeStatusKey(nodeID int32) proto.Key {
// MakeNameMetadataKey returns the key for the namespace.
func MakeNameMetadataKey(parentID uint32, name string) proto.Key {
- k := make([]byte, 0, len(NameMetadataPrefix)+maxVarintSize+len(name))
- k := make([]byte, 0, len(NameMetadataPrefix)+binary.MaxVarintLen64+len(name))
The use of binary.PutVarint in util/encoding.Encode should really be
removed. Looks like the only use of that method is by engine.Increment()
which appears to be unused.—
Reply to this email directly or view it on GitHub
https://github.com/cockroachdb/cockroach/pull/1356/files#r32524472.
|
LGTM modulo the |
| @@ -287,6 +287,11 @@ func DecodeVarintDecreasing(b []byte) ([]byte, int64) { | |||
| return leftover, ^v | |||
| } | |||
|
|
|||
| const ( | |||
| // MaxUvarintSize is the maximum size in bytes of an encoded uvarint. | |||
| MaxUvarintSize = 10 | |||
|
LGTM |
| @@ -62,7 +58,7 @@ func NodeStatusKey(nodeID int32) proto.Key { | |||
|
|
|||
| // MakeNameMetadataKey returns the key for the namespace. | |||
| func MakeNameMetadataKey(parentID uint32, name string) proto.Key { | |||
| k := make([]byte, 0, len(NameMetadataPrefix)+maxVarintSize+len(name)) | |||
| k := make([]byte, 0, len(NameMetadataPrefix)+encoding.MaxUvarintSize+len(name)) | |||
There was a problem hiding this comment.
disregard, encoding is util/encoding!
Keys for the table data