Skip to content
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

Include Corrupt key info in ParseInternalKey() #7497

Closed
ramvadiv opened this issue Oct 2, 2020 · 1 comment
Closed

Include Corrupt key info in ParseInternalKey() #7497

ramvadiv opened this issue Oct 2, 2020 · 1 comment
Assignees

Comments

@ramvadiv
Copy link
Contributor

ramvadiv commented Oct 2, 2020

Follow-up task from PR #7457 for an item requested by @ajkr

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.

@ramvadiv ramvadiv self-assigned this Oct 2, 2020
@ajkr
Copy link
Contributor

ajkr commented Oct 3, 2020

As discussed, currently the ParseInternalKey() call sites pretty much ignore the returned Status and handle failure in their own way. After we change those call sites to use the Status from ParseInternalKey(), they may have additional info to incorporate into the Status before returning it. Here is an example of how to compose Status messages:

s = Status::Corruption(prefix, s.getState());

facebook-github-bot pushed a commit that referenced this issue Oct 28, 2020
Summary:
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:
- 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: #7515

Reviewed By: ajkr

Differential Revision: D24240264

Pulled By: ramvadiv

fbshipit-source-id: bc48f5d4475ac19d7713e16df37505b31aac42e7
codingrhythm pushed a commit to SafetyCulture/rocksdb that referenced this issue Mar 5, 2021
)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants