Remove invalid assertion in compaction_picker_universal.cc #7421
Conversation
| // of files in bottommost level can be set to 0 to help | ||
| // compression. As a result, the following assert may not hold | ||
| // if the prev_smallest_seqno is 0. | ||
| assert(prev_smallest_seqno > largest_seqno); | ||
| } |
There was a problem hiding this comment.
Or should we change it to:
assert(largest_seqno == 0 || prev_smallest_seqno > largest_seqno);
There was a problem hiding this comment.
prev_smallest_seqno > largest_seqno wouldn't hold even for non-bottommost levels if there's a delete triggered compaction.
There was a problem hiding this comment.
sorry, just a question, what is delete triggered compaction? is it CompactionReason.kTtl:
rocksdb/include/rocksdb/listener.h
Lines 92 to 93 in c268628
There was a problem hiding this comment.
delete triggered compaction
Formally, delete triggered compaction (DTC) is using this feature (
rocksdb/include/rocksdb/utilities/table_properties_collectors.h
Lines 15 to 19 in 510c66f
But we also sometimes say DTC when referring to the underlying mechanism that supports it. That underlying mechanism "marks" files for compaction when they're generated and includes the mark in the file's manifest entry. The compaction picking algorithm can pick marked files for compaction regardless of everything else (e.g., whether the level score is large enough to need compaction).
There was a problem hiding this comment.
@anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
anand1976
force-pushed
the
compaction_picker
branch
from
1b343fc
to
27fb620
Compare
|
@anand1976 has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@anand1976 merged this pull request in 12ede5e. |
…7421) Summary: The assertion checks that there is no overlap in sequence numbers across levels in universal compaction. However, this assumption doesn't hold when there is a delete triggered compaction or a trivial move, as they operate on a subset of a level. Tests - make check Pull Request resolved: facebook#7421 Reviewed By: ajkr Differential Revision: D23872672 Pulled By: anand1976 fbshipit-source-id: c386deab8e01a5746ca996ff1f4ebcae3b15b7d2
…#12284) Summary: While ingesting multiple external files with key range overlap, current flow go through the lsm tree to do a search for a target level and later discard that result by defaulting back to L0. This PR improves this by just skip the search altogether. The other change is to remove default to L0 for the combination of universal compaction + force global sequence number, which was initially added to meet a pre #7421 invariant. Pull Request resolved: #12284 Test Plan: Added unit test: ./external_sst_file_test --gtest_filter="*IngestFileWithGlobalSeqnoAssignedUniversal*" Reviewed By: ajkr Differential Revision: D53072238 Pulled By: jowlyzhang fbshipit-source-id: 30943e2e284a7f23b495c0ea4c80cb166a34a8ac
The assertion checks that there is no overlap in sequence numbers across levels in universal compaction. However, this assumption doesn't hold when there is a delete triggered compaction or a trivial move, as they operate on a subset of a level.
Tests -
make check