internal/keyspan: add Value to Span#1382
Conversation
|
This change is |
nicktrav
left a comment
There was a problem hiding this comment.
Just nits.
Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jbowens and @sumeerbhola)
internal/keyspan/fragmenter_test.go, line 20 at r1 (raw file):
`(\d+):\s*(\w+)-*(\w+) *([^\n]*)`
nit: can the whitespace character after the third capture group be an \s? Slightly more explicit. i.e.:
(\d+):\s*(\w+)-*(\w+)\s*([^\n]*)
internal/keyspan/testdata/fragmenter_values, line 71 at r1 (raw file):
2: a---e banana 1: a-----g coconut flush-to d
I had to squint to spot the differences between this test case and the one prior (flush vs. truncate-and-flush). Might be worth a brief comment above one or both of the test cases to call out the difference explicitly.
Also, unrelated to the test case itself, and more to the docs on FlushTo, I was a little confused as to why the c-e spans were flushed. Reading inside the function itself, it seems that we "flush all span fragments with a start key <= key". Can we repeat that comment on the function signature itself?
internal/keyspan/testdata/fragmenter_values, line 85 at r1 (raw file):
1: e-g coconut 3: g-i orange
supernit: extra trailing newline
Add a Value to the `Span` type, copying it to both fragments at a fragmentation point. This will be used in the implementation of range keys, which will be fragmented but carry a value.
jbowens
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status: 13 of 16 files reviewed, 2 unresolved discussions (waiting on @nicktrav and @sumeerbhola)
internal/keyspan/fragmenter_test.go, line 20 at r1 (raw file):
Previously, nicktrav (Nick Travers) wrote…
`(\d+):\s*(\w+)-*(\w+) *([^\n]*)`nit: can the whitespace character after the third capture group be an
\s? Slightly more explicit. i.e.:(\d+):\s*(\w+)-*(\w+)\s*([^\n]*)
Nice, done.
internal/keyspan/testdata/fragmenter_values, line 71 at r1 (raw file):
Previously, nicktrav (Nick Travers) wrote…
I had to squint to spot the differences between this test case and the one prior (flush vs. truncate-and-flush). Might be worth a brief comment above one or both of the test cases to call out the difference explicitly.
Also, unrelated to the test case itself, and more to the docs on
FlushTo, I was a little confused as to why thec-espans were flushed. Reading inside the function itself, it seems that we "flush all span fragments with a start key <= key". Can we repeat that comment on the function signature itself?
Good call, done.
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 12 of 16 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions
internal/keyspan/testdata/fragmenter, line 25 at r2 (raw file):
1: j--m 2: m-----s 1: m-----s------z
why did this change?
jbowens
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions
internal/keyspan/testdata/fragmenter, line 25 at r2 (raw file):
Previously, sumeerbhola wrote…
why did this change?
The deleted portion ------z was ignored by the parsing regex and nonsensical here, where each line indicates 1 new fragment.
Add a Value to the
Spantype, copying it to both fragments at afragmentation point. This will be used in the implementation of range
keys, which will be fragmented but carry a value.