internal/testkeys: new package#1367
Conversation
|
This change is |
nicktrav
left a comment
There was a problem hiding this comment.
Reviewed 2 of 4 files at r1, all commit messages.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @jbowens and @sumeerbhola)
go.mod, line 11 at r1 (raw file):
github.com/ghemawat/stream v0.0.0-20171120220530-696b145b53b9 github.com/golang/snappy v0.0.3 github.com/klauspost/compress v1.12.3
I presume these are from adding strutil?
Wondering if we can could just inline the function (with appropriate attribution), rather than pulling in another dep (looks like it has a ton of transitive deps, based on the go.sum diff)? Given we're adding a "test-only" package, it seems heavy handed to pull in all these other dependencies.
With that to one side, if we take the path of adding the new dep, do we need to regen the vendor directory?
nicktrav
left a comment
There was a problem hiding this comment.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @jbowens and @sumeerbhola)
internal/testkeys/testkeys.go, line 137 at r1 (raw file):
// Slice returns the sub-keyspace from index i, inclusive, to index j // exclusive. Slice(i, j int) Keyspace
nit: Wondering if these doc strings should indicate that a new Keyspace is returned (for Slice and EveryN)?
Looks like alphabet returns a mutated copy of the receiver. I guess you could also have a implementation that returns a pointer to self and mutates, though that would seem weird.
Add an internal testkeys package for generating and comparing human-readable test keys. These test keys may be suffixed with an integer timestamp. The testkeys package exposes its own Comparer implementation that compares timestamps based on their logical value, rather than their byte encoding. As Pebble introduces new suffix-aware features, like range keys, we will increasingly have a need for human-readable suffixed keys. The key generation facilities in this package support generating keys of varying lengths interleaved. This is anticipated to be useful when testing or benchmarking compactions, where a subset of keys are overwritten and a subset are new.
nicktrav
left a comment
There was a problem hiding this comment.
Neat. Just some nits.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @jbowens, @nicktrav, and @sumeerbhola)
internal/testkeys/testkeys.go, line 226 at r2 (raw file):
} func keyCount(n, l int) int {
nit: could this be uint64 to squeeze as much out of the range as possible?
Also - does it make sense if l > n? In that case, do we need to cap l to be min(l, n)? Or is it a programmer error if l > n, in which case panic.
jbowens
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @nicktrav and @sumeerbhola)
internal/testkeys/testkeys.go, line 226 at r2 (raw file):
Previously, nicktrav (Nick Travers) wrote…
nit: could this be
uint64to squeeze as much out of the range as possible?Also - does it make sense if
l > n? In that case, do we need to caplto bemin(l, n)? Or is it a programmer error ifl > n, in which casepanic.
Yeah, it does still make sense if l > n, because l controls the maximum number of slots and n controls the number of characters that can fill a slot.
Add an internal testkeys package for generating and comparing human-readable
test keys. These test keys may be suffixed with an integer timestamp. The
testkeys package exposes its own Comparer implementation that compares
timestamps based on their logical value, rather than their byte encoding.
As Pebble introduces new suffix-aware features, like range keys, we will
increasingly have a need for human-readable suffixed keys.
The key generation facilities in this package support generating keys of
varying lengths interleaved. This is anticipated to be useful when testing or
benchmarking compactions, where a subset of keys are overwritten and a subset
are new.