storage/metamorphic: Add MVCC delete range using tombstone#83945
storage/metamorphic: Add MVCC delete range using tombstone#83945craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
erikgrinaker
left a comment
There was a problem hiding this comment.
It's a bit unclear to me how these tests verify the correctness of operations. Are we only testing that they don't crash or error?
I think what we really need is a set of randomized tests that verify that MVCC operations (including iterators) give the expected results, since there are far more edge cases than we can reasonably cover with explicit test cases.
| writer readWriterID | ||
| key roachpb.Key | ||
| endKey roachpb.Key | ||
| txn txnID |
There was a problem hiding this comment.
MVCC range tombstones can't be transactional, since we don't have ranged intents yet. Is this a required artifact of the test harness? If not, I think I'd prefer to avoid involving transactions.
There was a problem hiding this comment.
Good catch. I removed the transactional code; I figured we needed it in place to be able to fake latching in the operation generator, but turns out it's not necessary just yet.
e984862 to
ec8132b
Compare
It's basically a determinism test. We spin up Pebble instances with different options, throw in some random store restarts in and random other mvcc operations (including MVCC Puts, CPuts, Gets, iterators, etc) and ensure that the result of those operations is consistent across different runs and instances of Pebble with different options. If, say, a compaction is corrupting a key somewhere or is leading to lack of determinism or consistent results, it'd show up here. Errors don't fail the test if every instance of Pebble returned the same error when the same set of operations is run. |
erikgrinaker
left a comment
There was a problem hiding this comment.
Gotcha, thanks. Sounds like we're going to need some other test infrastructure for randomized correctness testing then. Will give it some thought.
This change adds MVCCDeleteRangeUsingTombstone to the MVCC metamorphic tests. MVCCDeleteRange had already existed in this test suite, so this ended up being a relatively simple addition. One part of cockroachdb#82545, with possibly more parts to follow as other MVCC-level operations are added that utilize `writer.{Put,Clear}{MVCC,Engine}RangeKey`. Release note: None.
ec8132b to
9901620
Compare
nicktrav
left a comment
There was a problem hiding this comment.
Nice!
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker)
|
TFTRs! bors r=erikgrinaker |
|
Build succeeded: |
jbowens
left a comment
There was a problem hiding this comment.
To improve coverage, I'm wondering if we some runs should perform the delete range as a scan with point deletes, in order to test equivalence between MVCCDeleteRange and MVCC point deletes?
Reviewable status:
They're not equivalent e.g. to an MVCC iterator, which will see range tombstones rather than point tombstones. They're only equivalent to an MVCC scan/get without tombstones. |
|
Yeah, makes sense. I wonder if we'd get more value out of MVCC metamorphic tests that perform equivalent operations adding abstractions on top, when necessary. Varying Pebble options is great for surfacing bugs in Pebble, but it seems unlikely to surface new MVCC bugs. |
This change adds MVCCDeleteRangeUsingTombstone to the MVCC
metamorphic tests. MVCCDeleteRange had already existed in
this test suite, so this ended up being a relatively simple
addition.
One part of #82545, with possibly more parts to follow
as other MVCC-level operations are added that utilize
writer.{Put,Clear}{MVCC,Engine}RangeKey.Release note: None.