storage_test: test incremental iteration via ExportToSST #47331
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
dt
changed the title
dt
force-pushed
the
inc-iter-test-via-export
branch
from
d8b7170 to
b6a8cc3
Compare
|
@pbardea This sadly doesn't catch either of the Seek bugs from last month, presumably since with so few keys we never get into cases where we can seek, but it seemed like maybe a start in terms of getting some test coverage that the c++ version? |
|
❌ The GitHub CI (Cockroach) build has failed on b6a8cc3e. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan. |
pbardea
approved these changes
Apr 13, 2020
| @@ -115,7 +196,7 @@ func TestMVCCIncrementalIterator(t *testing.T) { | |||
| testValue3 = []byte("val3") | |||
| testValue4 = []byte("val4") | |||
|
|
|||
| tsMin = hlc.Timestamp{WallTime: 0, Logical: 0} | |||
| tsMin = hlc.Timestamp{WallTime: 0, Logical: 1} | |||
There was a problem hiding this comment.
I was a bit confused about why we needed to up the logical timestamp for the export tests. I think it's odd enough that it may warrant a brief explanation as a comment?
TestMVCCIncrementalIterator used to serve as one of our primary unit tests on incremental iteration. However with the introduction of the c++ implementation of an incremental iterator, the fact that it directly tests the Go iterator leaves the parallel implementation (that is used by default at runtime) untested. This change extends the test to also verify each test case via engine.ExportToSST, which, because it lets the engine in use decide which iterator to use, means that it now tests both iterators. Additionally this serves to actually test that the usage of those iterators yields the expected results at the same time. Futhermore, both of these iterators behave differently when the min/max timestamp hints are provided, when they use a second filtered (time-bound) iterator to skip keys. Previously this test never set these hints, and thus never exercise the TBI path in either iterator. Not however that even with this hint set and the pair of iterators being used, this test still only covers a fraction of the TBI code, as the handful of keys used in each test never get flushed, much less into separate SSTs with different time-bounds, so all of the logic around Seeking over large swaths of keys, catching up in the known edges cases, etc is still uncovered. Release note: none.
dt
force-pushed
the
inc-iter-test-via-export
branch
from
b6a8cc3 to
9e93ac3
Compare
|
TFTR! |
Build failed (retrying...) |
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TestMVCCIncrementalIterator used to serve as one of our primary unit
tests on incremental iteration. However with the introduction of the c++
implementation of an incremental iterator, the fact that it directly
tests the Go iterator leaves the parallel implementation (that is used
by default at runtime) untested.
This change extends the test to also verify each test case via
engine.ExportToSST, which, because it lets the engine in use decide
which iterator to use, means that it now tests both iterators.
Additionally this serves to actually test that the usage of those
iterators yields the expected results at the same time.
Futhermore, both of these iterators behave differently when the min/max
timestamp hints are provided, when they use a second filtered
(time-bound) iterator to skip keys. Previously this test never set these
hints, and thus never exercise the TBI path in either iterator. Not
however that even with this hint set and the pair of iterators being
used, this test still only covers a fraction of the TBI code, as the
handful of keys used in each test never get flushed, much less into
separate SSTs with different time-bounds, so all of the logic around
Seeking over large swaths of keys, catching up in the known edges cases,
etc is still uncovered.
Release note: none.