-
Notifications
You must be signed in to change notification settings - Fork 895
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
Support Version Rollbacks for RocksDB #6
Support Version Rollbacks for RocksDB #6
Conversation
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
9237731
to
55e3747
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RatanRSur could you please squash this into one commit and rebase on the tip of master? thanks
No worries! I believe that once you mark your comment as resolved, the change requested block will be lifted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the turbulence!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore me
One more thing - can you withdraw your approval? We use those for signaling that someone has done a complete code review and thinks the changes are ready to be merged. |
Tell me what to click and where. I can't find an option for me to dismiss my own review |
I had to look it up myself! Turns out I can do it myself but so that you know: Where it says 1 approval, the drop down will have your name. On the right side overflow menu there's a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cause of Privacy AT failures are the RocksDBKeyValuePrivacyFactory
attempting to use the same RocksDB file as the RocksDBKeyValueFactory
.
Overriding of the storage path to include an additional path especially for the Privacy RocksDB file (yes, there are currently two RocksDB databases)
I've pre-emptively approved this, because as soon as you get those Privacy ATs passing (which could be done by accepting the suggested commits) then the PR is good to go!
...java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java
Show resolved
Hide resolved
...java/org/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValueStorageFactory.java
Outdated
Show resolved
Hide resolved
...g/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValuePrivacyStorageFactory.java
Outdated
Show resolved
Hide resolved
...g/hyperledger/besu/plugin/services/storage/rocksdb/RocksDBKeyValuePrivacyStorageFactory.java
Outdated
Show resolved
Hide resolved
Co-Authored-By: CJ Hare <CjHare@users.noreply.github.com> Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
403013b
to
6e5c662
Compare
Signed-off-by: CJ Hare <chris.hare@consensys.net> Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
PR description
Primary:
This fixes an issue where rolling back the default database version could cause an error. It's important that the loading logic depends only on the database version so we can have full control. It's still fine to set auxiliary properties like isSegmentIsolationSupported as we go through the creation process.
Other:
Extend the plugin interface with Closeable
Fixed Issue(s)