Fix range tombstones written to more files than necessary #4592
Conversation
|
I'll try to come up with a DB-level test later. |
There was a problem hiding this comment.
ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
LGTM The CockroachDB test which caught this is https://github.com/cockroachdb/cockroach/blob/master/pkg/storage/engine/rocksdb_test.go#L1267-L1398. It doesn't use anything super complicated, but it is written in Go. The slightly non obvious part is that our wrapper around The test starts by creating 3 sstables at L6 (using ingestion): It then writes the key |
|
@ajkr has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
New DB test LGTM |
|
appveyor error: |
|
@ajkr has updated the pull request. Re-import the pull request |
db/db_range_del_test.cc
Outdated
| for (const auto& name_and_table_props : all_table_props) { | ||
| num_range_deletions += name_and_table_props.second->num_range_deletions; | ||
| } | ||
| ASSERT_EQ(1, num_range_deletions); |
There was a problem hiding this comment.
It would be nice if you could assert the range deletion was present in the second L1 sstable, though I'd only do that if it is relatively easy.
| ASSERT_OK(Put("a", "val")); | ||
| ASSERT_OK(db_->DeleteRange(WriteOptions(), db_->DefaultColumnFamily(), | ||
| "c" + Key(1), "d")); | ||
| ASSERT_OK(Put("c" + Key(1), "value")); |
There was a problem hiding this comment.
Might want to add a comment that putting the key c gives the compaction output heuristics a stopping point. When #3977 is addressed this shouldn't be necessary.
| }); | ||
| ASSERT_EQ("a", l1_metadata[0].smallestkey); | ||
| ASSERT_EQ("a", l1_metadata[0].largestkey); | ||
| ASSERT_EQ("c" + Key(1), l1_metadata[1].smallestkey); |
There was a problem hiding this comment.
FWIW, this and the below line indirectly verify the range tombstone is in the second L1 SST.
There was a problem hiding this comment.
Well, this could be the the point mutation, but you're correct that the line below implies that the tombstone is in this sstable.
|
@ajkr has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
ajkr is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
) Summary: When there's a gap between files, we do not need to output tombstones starting at the next output file's begin key to the current output file. Pull Request resolved: facebook#4592 Differential Revision: D12808627 Pulled By: ajkr fbshipit-source-id: 77c8b2e7523a95b1cd6611194144092c06acb505
…elete-range Fix range tombstones written to more files than necessary (facebook#4592)
) Summary: When there's a gap between files, we do not need to output tombstones starting at the next output file's begin key to the current output file. Pull Request resolved: facebook#4592 Differential Revision: D12808627 Pulled By: ajkr fbshipit-source-id: 77c8b2e7523a95b1cd6611194144092c06acb505
When there's a gap between files, we do not need to output tombstones starting at the next output file's begin key to the current output file.
Test Plan:
make check -j64