storage: use SetOptions() when reusing Pebble iterators#81242
storage: use SetOptions() when reusing Pebble iterators#81242craig[bot] merged 1 commit intocockroachdb:masterfrom
SetOptions() when reusing Pebble iterators#81242Conversation
|
This change is |
18cdc6e to
87eb356
Compare
jbowens
left a comment
There was a problem hiding this comment.
Heads up that while cockroachdb/pebble#1675 merged, the Pebble bump is blocked on resolving a breaking code change to vfs.WithDiskHealthChecks, which in turn is blocked on cockroachdb/pebble#1690. 😵
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @nicktrav and @sumeerbhola)
87eb356 to
39cb5db
Compare
00c73da to
3c2f325
Compare
Iterator reuse now relies on `Pebble.SetOptions()` to configure the reused Pebble iterator. This allows a wider range of iterators to be reused, since previously only the bounds could be changed on existing iterators. Release note: None
3c2f325 to
0a00291
Compare
|
This should be good to go now, PTAL. A quick |
jbowens
left a comment
There was a problem hiding this comment.
I think we can merge this anyway, and work on performance optimizations later
Sounds good. I'll look at cockroachdb/pebble#1709 this week, which I think might make a difference here.
Reviewed 4 of 5 files at r4, 4 of 14 files at r5, all commit messages.
Reviewable status:
pkg/storage/pebble_iterator.go line 117 at r5 (raw file):
} else { var err error if p.iter, err = iterToClone.Clone(); err != nil {
Just a fyi, I filed cockroachdb/pebble#1712 so that we can avoid the SetOptions call in this case.
|
Sounds good, TFTR! bors r=jbowens |
|
Build succeeded: |
Iterator reuse now relies on
Pebble.SetOptions()to configure thereused Pebble iterator. This allows a wider range of iterators to be
reused, since previously only the bounds could be changed on existing
iterators.
Release note: None