storage/engine: fix pebbleBatch.iter reuse #41896
Conversation
|
This change is |
|
Hrmm, this is still broken. Now in a different way. Hold off on reviewing. |
`pebbleBatch.NewIterator` was setting `pebbleBatch.iter.inuse = true`, and then calling `pebbleIterator.init` which was clearing that field. This was broken by cockroachdb#41859 which refactored how `pebbleBatch` iterator reuse works. Added separate cached prefix and normal iterators to `pebble{Batch,ReadOnly}`. Various bits of higher-level code expect to be able to have a prefix and normal iterator open at the same time. In particularly, this comes up during intent resolution. This also mimics our usage of RocksDB which seems desirable in the short term even though the semantics of having two cached iterators is slightly odd. Fixes cockroachdb#41899
petermattis
force-pushed
the
pmattis/pebble-batch-iterator
branch
from
09131bf to
550d483
Compare
There was a problem hiding this comment.
Ok, all fixed up. This is ready for a look.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @itsbilal and @sumeerbhola)
pkg/storage/engine/pebble_batch.go, line 27 at r1 (raw file):
buf []byte prefixIter pebbleIterator normalIter pebbleIterator
@itsbilal I had to go ahead and do this because TestMVCCOpLogWriter/pebble was failing.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1.
Reviewable status:
pkg/storage/engine/pebble_batch.go, line 27 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
@itsbilal I had to go ahead and do this because
TestMVCCOpLogWriter/pebblewas failing.
Sounds good, thanks for doing this!
|
TFTR! |
pebbleBatch.NewIteratorwas settingpebbleBatch.iter.inuse = true,and then calling
pebbleIterator.initwhich was clearing thatfield. This was broken by #41859 which refactored how
pebbleBatchiterator reuse works.
Fixes #41899