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.

Sign up for GitHub

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

Jump to bottom

rocksdb: add key order check and turn off force-consistency-checks #8788

Merged
merged 5 commits into from
Oct 14, 2020
Merged

rocksdb: add key order check and turn off force-consistency-checks #8788

merged 5 commits into from
Oct 14, 2020

Conversation

yiwu-arbug
Copy link

@yiwu-arbug yiwu-arbug commented Oct 8, 2020 •
edited
Loading

Signed-off-by: Yi Wu [email protected]

What problem does this PR solve?

Problem Summary: The force-consistency-checks was set by default to investigate #8243 and prevent it from corrupt data further. However, the config add significant latency to writes (up to hundreds of ms) by holding global mutex. Despite effort in #8519 to reduce its performance impact, we still see it hurt performance in cases. Now that we learned the root cause of #8243, let's disable it.

What is changed and how it works?

What's Changed: disable force-consistency-checks

Also update rocksdb to include key order check:

488fb41 2020-10-11 66930949+ti-srebot.. rocksdb: Add compaction order check and data block cache check (#546)

Related changes

previous effort to optimize the config's performance: #8243

Check List

Tests: CI

Side effects: For user vulnerable to #8243 (with newer kernel version, >= 4.15, depend on distribution), without force-consistency-checks it may cause data corruption.

Release note

@yiwu-arbug yiwu-arbug added component/rocksdb Component: RocksDB engine sig/engine SIG: Engine needs-cherry-pick-release-4.0 Type: Need cherry pick to release 4.0 labels Oct 8, 2020
@gregwebs
Copy link
Contributor

gregwebs commented Oct 8, 2020

Did this configuration actually prevent corruption or just reduce it?

@yiwu-arbug
Copy link
Author

@gregwebs it does not prevent corruption, but it can fail TiKV when corruption is detected and prevent the corruption from propagate.

fix test
a5afab3 
Signed-off-by: Yi Wu <[email protected]>
@gregwebs
Copy link
Contributor

gregwebs commented Oct 9, 2020

Can we print a warning if the kernel version is newer? In the warning we can state that this can be configured.

@yiwu-arbug
Copy link
Author

Can we print a warning if the kernel version is newer? In the warning we can state that this can be configured.

we are adding a check to detect the corruption in a better way: tikv/rocksdb#195 and we expected we will get a fix from upstream soon.

Connor1996
Connor1996 approved these changes Oct 13, 2020
View reviewed changes
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Would you mind updating rust-rocksdb to include tikv/rust-rocksdb#546 by the way

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 13, 2020
@yiwu-arbug
Copy link
Author

LGTM. Would you mind updating rust-rocksdb to include tikv/rust-rocksdb#546 by the way

done

@yiwu-arbug yiwu-arbug changed the title rocksdb: turn off force-consistency-checks by default rocksdb: add key order check and turn off force-consistency-checks Oct 13, 2020
fix test
744ad47 
Signed-off-by: Yi Wu <[email protected]>
@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Oct 14, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Oct 14, 2020
@yiwu-arbug
Copy link
Author

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 14, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 1f5504f into tikv:master Oct 14, 2020
ti-srebot pushed a commit to ti-srebot/tikv that referenced this pull request Oct 14, 2020
@ti-srebot
cherry pick tikv#8788 to release-4.0
700414f 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Oct 14, 2020
Closed
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #8823

@yiwu-arbug yiwu-arbug deleted the conf_consistency branch October 15, 2020 21:24
@Connor1996 Connor1996 mentioned this pull request Nov 12, 2020
Merged
Connor1996 pushed a commit to Connor1996/tikv that referenced this pull request Nov 12, 2020
@Connor1996
rocksdb: add key order check and turn off force-consistency-checks (t…
b178f88 
…ikv#8788)

Signed-off-by: Yi Wu <[email protected]>
Signed-off-by: Connor <[email protected]>
ti-srebot pushed a commit that referenced this pull request Nov 23, 2020
@Connor1996
rocksdb: Fix cache key collision and turn off force-consistency-checks (
5e1bf17 
#8788) (#9029)

Signed-off-by: Yi Wu <[email protected]>
Signed-off-by: Connor <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Connor1996 Connor1996 Connor1996 approved these changes

@tabokie tabokie tabokie approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
component/rocksdb Component: RocksDB engine needs-cherry-pick-release-4.0 Type: Need cherry pick to release 4.0 sig/engine SIG: Engine status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yiwu-arbug @gregwebs @ti-srebot @Connor1996 @tabokie