rditer: replace ReplicaDataEngineIterator with IterateReplicaKeySpans
#84070
Conversation
|
This change is |
erikgrinaker
force-pushed
the
rditer-generator
branch
2 times, most recently
from
f8f533a to
ec16753
Compare
erikgrinaker
changed the title
ReplicateDataEngineIterator with IterateReplicaKeySpansReplicaDataEngineIterator with IterateReplicaKeySpans
erikgrinaker
force-pushed
the
rditer-generator
branch
from
ec16753 to
c1f6a4a
Compare
I think it's fine. We don't need range keys in every test, as long as we have sufficient test coverage, which I think we do (at least for them being included in snapshots) -- see e.g. integration tests in |
erikgrinaker
force-pushed
the
rditer-generator
branch
from
c1f6a4a to
55bcbd2
Compare
This patch renames the concept "key range" to "key span", and the functions now return regular `roachpb.Span` instead of `rditer.KeyRange`. "Range" is already an overloaded term (I have used "range keys in the range's key ranges" in a sentence!), and "key span" is a more widely used term in the code. Release note: None
erikgrinaker
force-pushed
the
rditer-generator
branch
from
55bcbd2 to
b9868c2
Compare
|
TFTR! bors r=aliher1911 |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @erikgrinaker)
pkg/kv/kvserver/rditer/replica_data_iter.go line 366 at r4 (raw file):
// iterutil.StopIteration(). // // Must use a reader with consistent iterators.
With some resumption information in the visitor interface we could also in the future solve #75824
|
bors r- This leaks an iterator if the seek errors. |
|
Canceled. |
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/rditer/replica_data_iter.go line 366 at r4 (raw file):
Previously, sumeerbhola wrote…
With some resumption information in the visitor interface we could also in the future solve #75824
Good point. I'll pick that up shortly, it should be fairly trivial.
…ans` This patch replaces `ReplicaDataEngineIterator` with `IterateReplicaKeySpans()`. The Raft snapshot protocol requires an unusual iteration order: for each of the range's key spans, first send all point keys then send all range keys. This allows ingesting SSTs that only span each key span (rather than across wide swaths of the keyspace), with efficient, separate iteration over point/range keys. While the current `ReplicaDataEngineIterator` already does this, the iteration order is rather peculiar, which can be unexpected to users. `IterateReplicaKeySpans` attempts to make this iteration order more explicit. Other approaches were considered but rejected: * Use an iterator generator, which returns the next iterator in the sequence. This was very unergonomic to use, since it required nested iteration loops (one over the iterators, and one per iterator) which make variable naming/reuse and iterator closing awkward. * Use a visitor for each key instead of keyspan. This would again end up hiding the iteration sequence/structure. It also would make it impossible to skip across key spans, or do partial processing of a key span, since it could only abort the entire iteration. Release note: None
erikgrinaker
force-pushed
the
rditer-generator
branch
from
b9868c2 to
d789c46
Compare
|
bors r=aliher1911 |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
See #83747 for prior discussion.
rditer: rename "key range" to "key span" and use
roachpb.SpanThis patch renames the concept "key range" to "key span", and the
functions now return regular
roachpb.Spaninstead ofrditer.KeyRange."Range" is already an overloaded term (I have used "range keys in the
range's key ranges" in a sentence!), and "key span" is a more widely used
term in the code.
Release note: None
rditer: replace
ReplicaDataEngineIteratorwithIterateReplicaKeySpansThis patch replaces
ReplicaDataEngineIteratorwithIterateReplicaKeySpans().The Raft snapshot protocol requires an unusual iteration order: for each
of the range's key spans, first send all point keys then send all range
keys. This allows ingesting SSTs that only span each key span (rather
than across wide swaths of the keyspace), with efficient, separate
iteration over point/range keys.
While the current
ReplicaDataEngineIteratoralready does this, theiteration order is rather peculiar, which can be unexpected to users.
IterateReplicaKeySpansattempts to make this iteration order moreexplicit.
Other approaches were considered but rejected:
Use an iterator generator, which returns the next iterator in the
sequence. This was very unergonomic to use, since it required nested
iteration loops (one over the iterators, and one per iterator) which
make variable naming/reuse and iterator closing awkward.
Use a visitor for each key instead of keyspan. This would again end up
hiding the iteration sequence/structure. It also would make it
impossible to skip across key spans, or do partial processing of a key
span, since it could only abort the entire iteration.
Touches #82591.
Release note: None