This repository has been archived by the owner on Nov 6, 2020. It is now read-only.
Improve handling of RocksDB corruption #7630
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andresilva
added
A0-pleasereview 🤓
|
It looks like @andresilva signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
andresilva
force-pushed
the
rocksdb-corruption
branch
2 times, most recently
from
ea5f53f
to
cef3ddd
Compare
andresilva
force-pushed
the
rocksdb-corruption
branch
from
cef3ddd
to
79e1d02
Compare
andresilva
force-pushed
the
rocksdb-corruption
branch
from
df53656
to
740d466
Compare
debris
approved these changes
Jan 19, 2018
debris
added
A8-looksgood 🦄
kirushik
reviewed
Jan 19, 2018
util/kvdb-rocksdb/src/lib.rs
Outdated
| @@ -425,7 +454,11 @@ impl Database { | |||
| } | |||
| } | |||
| } | |||
| db.write_opt(batch, &self.write_opts)?; | |||
|
|
|||
| mark_corruption( | |||
There was a problem hiding this comment.
I would rename mark_corruption into something like check_for_corruption, otherwise it looks like you're uncondtionnaly marking the db as corrupted here.
kirushik
added
A6-mustntgrumble 💦
kirushik
approved these changes
Jan 19, 2018
kirushik
added
A8-looksgood 🦄
debris
pushed a commit
that referenced
this pull request
Jan 22, 2018
* kvdb-rocksdb: update rust-rocksdb version * kvdb-rocksdb: mark corruptions and attempt repair on db open * kvdb-rocksdb: better corruption detection on open * kvdb-rocksdb: add corruption_file_name const * kvdb-rocksdb: rename mark_corruption to check_for_corruption
Merged
debris
pushed a commit
that referenced
this pull request
Jan 22, 2018
* kvdb-rocksdb: update rust-rocksdb version * kvdb-rocksdb: mark corruptions and attempt repair on db open * kvdb-rocksdb: better corruption detection on open * kvdb-rocksdb: add corruption_file_name const * kvdb-rocksdb: rename mark_corruption to check_for_corruption
Merged
5chdn
pushed a commit
that referenced
this pull request
Jan 23, 2018
* Update .gitlab-ci.yml fix cache:key * Fixed delegatecall's from/to (#7568) * Fixed delegatecall's from/to, closes #7166 * added tests for delegatecall traces, #7167 * Fix Temporarily Invalid blocks handling (#7613) * Handle temporarily invalid blocks in sync. * Fix tests. * Improve handling of RocksDB corruption (#7630) * kvdb-rocksdb: update rust-rocksdb version * kvdb-rocksdb: mark corruptions and attempt repair on db open * kvdb-rocksdb: better corruption detection on open * kvdb-rocksdb: add corruption_file_name const * kvdb-rocksdb: rename mark_corruption to check_for_corruption
5chdn
pushed a commit
that referenced
this pull request
Jan 23, 2018
* Improve handling of RocksDB corruption (#7630) * kvdb-rocksdb: update rust-rocksdb version * kvdb-rocksdb: mark corruptions and attempt repair on db open * kvdb-rocksdb: better corruption detection on open * kvdb-rocksdb: add corruption_file_name const * kvdb-rocksdb: rename mark_corruption to check_for_corruption * Hardening of CSP (#7621) * Fixed delegatecall's from/to (#7568) * Fixed delegatecall's from/to, closes #7166 * added tests for delegatecall traces, #7167 * Light client RPCs (#7603) * Implement registrar. * Implement eth_getCode * Don't wait for providers. * Don't wait for providers. * Fix linting and wasm tests. * Problem: AttachedProtocols don't get registered (#7610) I was investigating issues I am having with Whisper support. I've enabled Whisper on a custom test network and inserted traces into Whisper handler implementation (Network<T> and NetworkProtocolHandler for Network<T>) and I noticed that the handler was never invoked. After further research on this matter, I found out that AttachedProtocol's register function does nothing: https://github.com/paritytech/parity/blob/master/sync/src/api.rs#L172 but there was an implementation originally: 99075ad#diff-5212acb6bcea60e9804ba7b50f6fe6ec and it did the actual expected logic of registering the protocol in the NetworkService. However, as of 16d84f8#diff-5212acb6bcea60e9804ba7b50f6fe6ec ("finished removing ipc") this implementation is gone and only the no-op function is left. Which leads me to a conclusion that in fact Whisper's handler never gets registered in the service and therefore two nodes won't communicate using it. Solution: Resurrect original non-empty `AttachedProtocols.register` implementation Resolves #7566 * Fix Temporarily Invalid blocks handling (#7613) * Handle temporarily invalid blocks in sync. * Fix tests.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #6959.
Maybe fixes #7334.
When writing to RocksDB if it returns a corruption error we create a
CORRUPTEDfile that will trigger a db repair when re-opening the database. Don’t know if the repair process will actually fix the corruption, even if it does it may do so at the cost of losing data and might cause random errors (e.g. we expect some data to exist in the db and it’s not there), if we see any of those in the future we can ask the users if there’s alostfolder in their RocksDB folder which indicates that aDB::Repairwas run. Also added detection for corruption like #7623.