pkg/storage: support suffix replacement on MVCC interval collector#76327
Conversation
|
This change is |
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @dt, @jbowens, and @nicktrav)
-- commits, line 4 at r1:
... added to Pebble ...
pkg/storage/pebble.go, line 386 at r1 (raw file):
// add collects the given slice in the collector. The slice may be an entire // encoded MVCC key, or the suffix of an encoded key. In the case of the latter, // the slice must include the sentinel byte.
The definition of the suffix does not include the sentinel byte as described in
cockroach/pkg/storage/engine_key.go
Lines 61 to 65 in 45d1c9e
Which is why we needed the code in
cockroach/pkg/storage/pebble.go
Lines 102 to 104 in 45d1c9e
The `SuffixReplaceableBlockCollector` interface was added to Pebble in cockroachdb/pebble#1377, which allows a block property collector to indicate that it supports being updated during suffix replacement. The existing `pebbleDataBlockMVCCTimeIntervalCollector` block property collector does not currently support suffix replacement. However, given a new suffix, the interval for a block can be trivially calculated. Implement the `SuffixReplaceableBlockCollector` interface. Release note: None
8003e66 to
130d69f
Compare
nicktrav
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, sumeerbhola wrote…
... added to Pebble ...
Done.
pkg/storage/pebble.go, line 386 at r1 (raw file):
Previously, sumeerbhola wrote…
The definition of the suffix does not include the sentinel byte as described in
cockroach/pkg/storage/engine_key.go
Lines 61 to 65 in 45d1c9e
Which is why we needed the code incockroach/pkg/storage/pebble.go
Lines 102 to 104 in 45d1c9e
Ah, missed that. I was using "suffix" in a more general "trailing bytes" sense. I didn't realize "suffix" has a precise definition in this context (MVCC timestamp + length byte; no sentinel).
If I'm understanding correctly now, this method should only receive either:
- a full
EngineKey(which will include the sentinel) when called fromAdd; or - a bare suffix (no sentinel), when called from
UpdateKeySuffixes
I added some error handling and some additional test cases.
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status:
|
TFTR! bors r=sumeerbhola |
|
Build failed (retrying...): |
|
Build succeeded: |
The
SuffixReplaceableBlockCollectorinterface was added to to Pebblein cockroachdb/pebble#1377, which allows a block property collector to
indicate that it supports being updated during suffix replacement.
The existing
pebbleDataBlockMVCCTimeIntervalCollectorblock propertycollector does not currently support suffix replacement. However, given
a new suffix, the interval for a block can be trivially calculated.
Implement the
SuffixReplaceableBlockCollectorinterface.Release note: None