Repository navigation
Conversation
bee344c to
df05720
Compare
Title(Describe updated until commit 75607c6)feat(db): add SeekLT and Last to iterators, rename Seek to SeekGE PR TypeEnhancement, Bug fix Description
|
| Relevant files | |||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Miscellaneous | 9 files
| ||||||||||||||||||
| Enhancement | 7 files
| ||||||||||||||||||
| Bug fix | 1 files
| ||||||||||||||||||
| Tests |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4268 +/- ##
==========================================
+ Coverage 81.50% 81.57% +0.07%
==========================================
Files 500 501 +1
Lines 36844 37034 +190
==========================================
+ Hits 30029 30211 +182
- Misses 6809 6817 +8
Partials 6 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Reviewed PR #4268 (feat(db): add SeekLT and Last to iterators, rename Seek to SeekGE). This is a clean, well-scoped refactor: Seek renamed to SeekGE everywhere, new SeekLT/Last added across all db.Iterator implementations (pebble, pebblev2, memory, remote), with the gRPC wire protocol updated consistently (SEEK→SEEK_GE same wire number, new SEEK_LT/LAST ops added) and the server-side cursor handling split into a moveCursor helper that correctly short-circuits on errors. Checked carefully for:
I found no correctness bugs, race conditions, panics, or resource leaks. The PR is clean — no inline comments posted. |
df05720 to
61075d7
Compare
|
Reviewed the PR (db iterator rename Seek→SeekGE, new SeekLT/Last across memory/pebble/pebblev2/remote backends, plus the gRPC proto/handler changes). The change is clean: memory's binary-search reimplementation of SeekGE/SeekLT matches pebble's semantics, all call sites were mechanically renamed (verified via repo-wide grep — no leftover |
| return i.Valid() | ||
| } | ||
|
|
||
| func (i *iterator) SeekLT(key []byte) bool { |
There was a problem hiding this comment.
Failed SeekLT + Prev behaves differently from pebble here. Pebble returns false, while memory turns it into First().
Probably, there are other differences in memory and pebble behaviour on such "out-of-range" cases, like SeekGE + Next etc.
There are no places in production where we use similar combinations of moves, but at least worth checking
There was a problem hiding this comment.
Good catch, thank you! I will go over it and see the places behavior deviates between DBs and try to make all behave same
There was a problem hiding this comment.
Maybe, we can add something similar to pebble's positioned flag to track this or strictly limit curInd
And different behaviour existed before this PR on other scenarios, SeekLT + Prev is the new one. So maybe we can fix it in other PR or leave it as it is. What do you think?
There was a problem hiding this comment.
Fixed. I added a positioned flag as you suggested, and curInd can now only go one step past either end, so moving further keeps the iterator exhausted. That covers the SeekLT + Prev case and an older one where SeekGE past the end + Next + Prev returned true with an empty key. A new iterator still moves to the first key on Next/Prev, same as pebble.
To check there weren't more, I ran every move sequence up to 3 steps long against memory, pebble, pebblev2 and remote (3 keys, 1 key, empty DB). They all match now. I also added a table of these cases to the shared suite in db/testutil.go, so all backends are tested against them.
While checking the rest of the API I found three other ways memory differs from pebble, none related to iterator moves:
- Batch DeleteRange deletes the keys that exist when it's called, while pebble applies the range when the batch is written. Keys written to the DB in between survive in memory.
- NewIterator(prefix, false) still stops at the prefix in memory, while pebble uses the prefix only as the starting point. No production code passes a prefix with false today.
- A prefix made only of 0xFF bytes returns nothing in memory, because its upper bound is nil and memory treats that as an empty bound instead of no bound.
These are older than this PR and unrelated to SeekLT/Last, so I'll fix them in a separate PR.
Expose pebble's SeekLT and Last on every backend and name Seek after what it does. The remote protocol renames SEEK to SEEK_GE and adds SEEK_LT and LAST.
61075d7 to
75607c6
Compare
|
Reviewed PR #4268 (db iterator SeekGE/SeekLT/Last refactor) across pebble, pebblev2, memory, and remote/gRPC implementations plus all call-site renames. Traced the memory iterator's new exhaustion-clamping logic against pebble semantics and verified all new edge-case tests. No correctness, concurrency, or resource-leak issues found; the change is a clean, mechanical, well-tested refactor. No inline comments posted — PR is clean. |
Seekwas always pebble'sSeekGE, so callers that wanted the last key below a bound had to seek forward and step back withPrev. This exposes pebble'sSeekLTandLastdirectly and namesSeekafter what it does.Changes
db.Iterator:Seek→SeekGE; newSeekLTandLast.SeekGE,SeekLT,Last.SEEK→SEEK_GE(same wire number), newSEEK_LTandLASTops; client positioning methods share one helper; the server's iterator ops move intomoveCursor, andCLOSEreturns early.SeekGE, no behavior change.Tests
SeekGE/SeekLTtable (exact, between, out of range, prefix bounds),Laston bounded and empty ranges, reverse iteration fromLast, direction changes afterSeekLT/Last, panics after close.SEEK_LTandLASTover a real gRPC server.