In ParseInternalKey(), include corrupt key info in Status #7515
Conversation
…cts to option to log data
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. |
db/dbformat.h
Outdated
| if (n < kNumInternalBytes) { | ||
| return Status::Corruption("Internal Key too small"); | ||
| std::string msg("Internal Key too small. "); | ||
| msg.append("Size=" + std::to_string(n) + ". "); |
There was a problem hiding this comment.
Use StringUtil::ToString().
There was a problem hiding this comment.
I don't understand this comment... Why?
|
@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. |
…param, rename pikStatus to pik_status, revert back to previous output format in DebugString()
|
@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. |
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.
| if (ParseInternalKey(iter->key(), &ikey) != Status::OK()) { | ||
| Status pik_status = | ||
| ParseInternalKey(iter->key(), &ikey, allow_data_in_errors); | ||
| if (!pik_status.ok()) { | ||
| // stop at corrupted key | ||
| if (assert_valid_internal_key_) { |
There was a problem hiding this comment.
[existing issue] It looks like this is initialized to false (
rocksdb/db/compaction/compaction_job.cc
Line 897 in a16d1b2
MergeHelper:: assert_valid_internal_key_ and also CompactionIterator::expect_valid_internal_key_ -- they seem error prone and not useful since we always want key corruptions to be reported.
There was a problem hiding this comment.
Agree. I will fix it in a separate PR.
|
@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. |
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.
1 similar comment
) Summary: Fixes Issue facebook#7497 When allow_data_in_errors db_options is set, log error key details in `ParseInternalKey()` Have fixed most of the calls. Have few TODOs still pending - because have to make more deeper changes to pass in the allow_data_in_errors flag. Will do those in a separate PR later. Tests: - make check - some of the existing tests that exercise the "internal key too small" condition are: dbformat_test, cuckoo_table_builder_test - some of the existing tests that exercise the corrupted key path are: corruption_test, merge_helper_test, compaction_iterator_test Example of new status returns: - Key too small - `Corrupted Key: Internal Key too small. Size=5` - Corrupt key with allow_data_in_errors option set to false: `Corrupted Key: '<redacted>' seq:3, type:3` - Corrupt key with allow_data_in_errors option set to true: `Corrupted Key: '61' seq:3, type:3` Pull Request resolved: facebook#7515 Reviewed By: ajkr Differential Revision: D24240264 Pulled By: ramvadiv fbshipit-source-id: bc48f5d4475ac19d7713e16df37505b31aac42e7
Fixes Issue #7497
When allow_data_in_errors db_options is set, log error key details in
ParseInternalKey()Have fixed most of the calls. Have few TODOs still pending - because have to make more deeper changes to pass in the allow_data_in_errors flag. Will do those in a separate PR later.
Tests:
Example of new status returns:
Corrupted Key: Internal Key too small. Size=5Corrupted Key: '<redacted>' seq:3, type:3Corrupted Key: '61' seq:3, type:3