Change ParseInternalKey() to return Status instead of bool #7457
Conversation
ramvadiv
changed the title
There was a problem hiding this comment.
@ramvadiv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@ramvadiv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@ramvadiv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@ramvadiv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
LGTM! There could probably be a follow-up task to combine this with #7420 by pushing down the option's value into ParseInternalKey() (defaulting to false when unknown) and then including the corrupt key in the Status::Corruption there.
The github convention for linking an issue is to put a line "Fixes #7430." in the description (rather than title), then it'll auto-close the issue when this PR merges.
db/range_del_aggregator.h
Outdated
| @@ -283,7 +283,7 @@ class RangeDelAggregator { | |||
|
|
|||
| bool ShouldDelete(const Slice& key, RangeDelPositioningMode mode) { | |||
| ParsedInternalKey parsed; | |||
| if (!ParseInternalKey(key, &parsed)) { | |||
| if (ParseInternalKey(key, &parsed) != Status::OK()) { | |||
There was a problem hiding this comment.
same, I think the return false should even be deleted.
There was a problem hiding this comment.
Thanks a lot for your review/comments Andrew. I have handled all of your other comments. I am not very clear about this point - Shouldn't we still return false in this case?
There was a problem hiding this comment.
I don't know. Returning false originally felt safe here because it conservatively preserves data that we cannot conclusively determine whether it's deleted due to a corruption. OTOH, it doesn't seem right to have a code path that makes a guess about whether data is deleted or not, even if it is the seemingly safer guess. The final solution should involve propagating the Status, so I guess it doesn't matter much what we do in the meantime since I don't think there are any good options (leave the return false; guess here, or let the code continue and probably crash).
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
ramvadiv
changed the title
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@ramvadiv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
…uted to circleci: build-linux-shared_lib-alt_namespace-status_checked test failure
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
…rator's constructor
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@ramvadiv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
…7457) Summary: Fixes facebook#7430 Change ParseInternalKey() to return Status instead of bool. db_bench (seekrandom) based before/after results with value size of 100 bytes and 16 bytes can be found at (tests ran on an udb server): https://www.dropbox.com/s/47bwamdy5ozngph/PIK_ret_Status_results.xlsx?dl=0  Pull Request resolved: facebook#7457 Reviewed By: ajkr Differential Revision: D24002433 Pulled By: ramvadiv fbshipit-source-id: ac253ecf577a29044c47c3fe254a01e71404c44c
Fixes #7430
Change ParseInternalKey() to return Status instead of bool.
db_bench (seekrandom) based before/after results with value size of 100 bytes and 16 bytes can be found at (tests ran on an udb server):
https://www.dropbox.com/s/47bwamdy5ozngph/PIK_ret_Status_results.xlsx?dl=0