-
Notifications
You must be signed in to change notification settings - Fork 6.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return Aborted when no valid input files are found for a CompactFile call #12633
Conversation
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
db/compaction/compaction_picker.cc
Outdated
return Status::Aborted( | ||
"There are no valid input files found for the specified files."); | ||
} | ||
assert(!smallestkey.empty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think empty user key is allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite get this, in the RangeOverlapWithCompaction
function, empty user key can cause assertions in this logic:
rocksdb/db/compaction/compaction_picker.cc
Line 266 in 9bddac0
ucmp->CompareWithoutTimestamp(smallest_user_key, |
So I think it's better to not pass empty boundaries to that function yet. These user key boundaries come from the existing files' boundaries. At this point, they should either represent some file's actual range, or no valid files found and they are empty. So here we want to not check further if range overlap when it's empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I didn't get that you meant Put("","")
is allowed. Thanks for catching this!
@@ -1040,6 +1042,13 @@ Status CompactionPicker::SanitizeCompactionInputFilesForAllLevels( | |||
} | |||
} | |||
} | |||
|
|||
if (!valid_input_files_found) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does checking is_first
works here? Or rename is_first
to valid_input_files_found
for better naming.
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Closing since #12628 will take care of this issue. |
Without this check,
smallestkey
andlargestkey
can be fed intoRangeOverlapWithCompaction
function while being empty boundaries. That function is not prepared to handle empty boundaries. If there are concurrent compactions ongoing, we can get some assertions.Test plan:
added unit test