Skip to content

c-deps: bump rocksdb for safer LSM invariant checks #40712

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

Merged

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Sep 12, 2019

Picks up cockroachdb/rocksdb#49.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

Don't we need to enable force_consistency_checks for this to have any effect?

@ajkr
Copy link
Contributor Author

ajkr commented Sep 12, 2019

Yes, I was planning to do it separately, though.

@petermattis
Copy link
Collaborator

Yes, I was planning to do it separately, though.

🤷‍♂an extra bors pass wouldn't be my choice.

@ajkr
Copy link
Contributor Author

ajkr commented Sep 12, 2019

They are somewhat independent - force_consistency_checks can be enabled with or without this PR. Without this PR it's less ideal because it prints to stderr then abort()s. That's less ideal than continuing and returning an error because buffered log messages will be lost (which are likely the ones describing the current compaction) and idk if the stderr is captured anywhere. At the same time, without this PR force_consistency_checks should still be effective in preventing the DB from going into a corrupt state.

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

Oops, didn't mean to hold up approval.

@ajkr
Copy link
Contributor Author

ajkr commented Sep 12, 2019

Plus doing it separately allows us to backport it to release-19.1 and release-2.1 using the backport tool, which we couldn't do if it includes a RocksDB version bump. Idk, I am also suffering from clicking around PRs too much, but can tolerate it for a little while longer.

@ajkr
Copy link
Contributor Author

ajkr commented Sep 12, 2019

bors r+

craig bot pushed a commit that referenced this pull request Sep 12, 2019
40712: c-deps: bump rocksdb for safer LSM invariant checks r=ajkr a=ajkr

Picks up cockroachdb/rocksdb#49.

Release note: None

Co-authored-by: Andrew Kryczka <[email protected]>
@craig
Copy link
Contributor

craig bot commented Sep 12, 2019

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants