feat: Add paranoid file checks in RocksDB - #3511
Conversation
fe96005 to
db830b0
Compare
db830b0 to
223fe40
Compare
|
This PR is stale because it has been open 14 days with no activity. It will be closed in 7 days unless you remove the |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 223fe40d5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rocksdb | ||
| .apply_env_options_to_cfs(cf_names) | ||
| .map_err(typed_store_err_from_rocks_err)?; |
There was a problem hiding this comment.
When reopening a database after an unclean shutdown, open_cf_descriptors can replay the WAL and flush recovery SSTs before it returns. Because paranoid_file_checks is applied only afterward, those SST writes bypass the requested checksum verification. Apply the environment override to each column family's Options before calling open_cf_descriptors so recovery and startup writes are covered too.
Useful? React with 👍 / 👎.
| // SAFETY: typed-store RocksDB tests use a process-wide mutex to serialize env mutation. | ||
| unsafe { | ||
| std::env::set_var(name, value); | ||
| } |
There was a problem hiding this comment.
When this test runs in parallel on Unix, the custom mutex does not make set_var safe because several tests call open_rocksdb without acquiring it, and default_db_options reads environment variables during those opens. Rust requires that no other thread read or write the environment while set_var runs, so this can race and invoke undefined behavior; run this case in an isolated subprocess or otherwise serialize every environment access in the test process.
Useful? React with 👍 / 👎.
halfprice
left a comment
halfprice
left a comment
There was a problem hiding this comment.
Thanks @sadhansood for the PR! The logic LGTM. Just one qq: why using env variable instead of config?
|
This PR is stale because it has been open 14 days with no activity. It will be closed in 7 days unless you remove the |
2 participants
Description
Add an option for paranoid file checks which lets RocksDB check block checksums after .sst file writes.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that
a user might notice and any actions they must take to implement updates. (Add release notes after the colon for each item)