storageccl: use the new PebbleIterator in ExternalSSTReader #84666
Conversation
|
This change is |
msbutler
force-pushed
the
butler-pebble-repro
branch
from
1b52ea2 to
c840968
Compare
msbutler
force-pushed
the
butler-pebble-repro
branch
from
c840968 to
6ffa08e
Compare
| var reader sstable.ReadableFile = raw | ||
| for i, basename := range filenames { | ||
| // prevent capturing the loop variables by reference when defining openAt below. | ||
| basename := basename |
There was a problem hiding this comment.
@jbowens if I comment these local vars out, the unit test I wrote in the PR still passes :(. You mentioned that the roachtest failures occur at block boundaries. I assume this occurs when pebbleIterator.Iter.ExternalFiles[i].ReadAt is called. Do you have any tips on how I could construct the SSTs in the unit test to ensure the test hits block boundaries? Maybe this is overkill, since we no know the bug is squashed and the test still adds good coverage.
There was a problem hiding this comment.
Does it trigger if you vary the number of keys per-file, eg, 10, 100, 1000?
There was a problem hiding this comment.
nope. I also changed the sst's to partially overlap, and still no repro. They currently look like this:
// t3 a500--------------------a10000
//
// t2 a50--------------a1000
//
// t1 a0----a100
9b2bb35 to
f2038ee
Compare
This PR refactors all call sites of ExternalSSTReader(), to support using the new PebbleIterator, which has baked in range key support. Most notably, this PR replaces the multiIterator used in the restore data processor with the PebbleSSTIterator. This patch is apart of a larger effort to teach backup and restore about MVCC bulk operations. Next, the readAsOfIterator will need to learn how to deal with range keys. Informs cockroachdb#71155 This PR addresses a bug created in cockroachdb#83984: loop variables in ExternalSSTReader were captured by reference, leading to roachtest failures (cockroachdb#84240, cockroachdb#84162). Informs #71155i Fixes: cockroachdb#84240, cockroachdb#84162, cockroachdb#84181 Release note: none
msbutler
force-pushed
the
butler-pebble-repro
branch
from
f2038ee to
85cd5ab
Compare
|
I've got a couple of PRs waiting for this to merge. So I'm going to merge this and file an issue for myself to further investigate a test that covers the capture by reference issue. |
|
bors r=erikgrinaker |
|
Build succeeded: |
This PR refactors all call sites of ExternalSSTReader(), to support using the
new PebbleIterator, which has baked in range key support. Most notably, this
PR replaces the multiIterator used in the restore data processor with the
PebbleSSTIterator.
This patch is apart of a larger effort to teach backup and restore about MVCC
bulk operations. Next, the readAsOfIterator will need to learn how to
deal with range keys.
Informs #71155
This PR addresses a bug created in #83984: loop variables in
ExternalSSTReader were captured by reference, leading to roachtest failures
(#84240, #84162).
Informs #71155
Fixes: #84240, #84162, #84181
Release note: none