db: support configuring iterator during Clone#1766
db: support configuring iterator during Clone#1766jbowens merged 1 commit intocockroachdb:masterfrom
Conversation
When creating an iterator through Clone, CockroachDB clones and then immediately calls SetOptions. This adds unnecessary key comparisons to compute the delta in the options, and if the options changed significantly, adds unnecessary iterator stack construction. Add a CloneOptions parameter to Clone that allows the user to optionally specify IterOptions to construct the iterator with. Additionally, add a field to support constructing the clone with a refreshed view of the indexed batch, when cloning an iterator over an indexed batch.
|
This change is |
jbowens
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion
iterator.go line 2118 at r1 (raw file):
// will cause an increase in memory and disk usage (use NewSnapshot for that // purpose). func (i *Iterator) Clone(opts CloneOptions) (*Iterator, error) {
I considered not changing this function's signature, and adding a new CloneWithOptions function, but we don't really provide any interface stability guarantees to external users. I'm a little wary of expanding the pebble public interface too much, but I don't feel strongly.
|
Thanks! Overall API looks good. Gave this a spin over in cockroachdb/cockroach#82981, but seems like the latest Pebble When using a batch, the iterator constructed in Will give this another go once |
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @erikgrinaker and @jbowens)
iterator.go line 2118 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
I considered not changing this function's signature, and adding a new
CloneWithOptionsfunction, but we don't really provide any interface stability guarantees to external users. I'm a little wary of expanding the pebble public interface too much, but I don't feel strongly.
I think this is fine personally. It has the advantage of making the options and their implications very visible too.
Ah, yeah, the fix is in #1753. |
nicktrav
left a comment
There was a problem hiding this comment.
Nice one.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @itsbilal and @jbowens)
jbowens
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @itsbilal and @jbowens)
When creating an iterator through Clone, CockroachDB clones and then
immediately calls SetOptions. This adds unnecessary key comparisons to compute
the delta in the options, and if the options changed significantly, adds
unnecessary iterator stack construction.
Add a CloneOptions parameter to Clone that allows the user to optionally
specify IterOptions to construct the iterator with. Additionally, add a field
to support constructing the clone with a refreshed view of the indexed batch,
when cloning an iterator over an indexed batch.
Close #1712.