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
Remove need for &mut self in create_cf and drop_cf (v2) #506
Conversation
tests/test_db.rs
Outdated
fn test_open_as_multi_threaded() { | ||
let primary_path = DBPath::new("_rust_rocksdb_test_open_as_multi_threaded"); | ||
|
||
let db = DbWithThreadMode::<MultiThreaded>::open_default(&primary_path).unwrap(); |
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.
After I wrote this, I realized there is some other compile-error message checks. ;) Maybe, should I write single thread version of this using that facility?
@aleksuss Hi, I fixed the CIs too. (latest commits CI aren't run for some reason). |
@ryoqun notify me please when you'll finish to modify the PR 😉 |
@aleksuss oh, thanks for paying close attention to this. I think I've addressed all of my self-review comments. So, I'm finished now. Also, I actually tried to use the new api at our project, which necessitated another somewhat large changes. For inspiration, please take a look for what would look like for the newer api change: solana-labs/solana#16227. Also, I checked 4 combination of (with/without multi-threaded-as-default, old/new api) by hand to confirm I broke nothing. So, this looks good? happy to address any new review comments. :) |
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.
Overall looks pretty good as 61f323b.
But couple nitpicks.
@aleksuss @stanislav-tkach thanks for careful reviews! I've fixed all nitpicks. Could you confirm them and merge if possible? Thanks for taking some time on this. :) |
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.
Well done ! 👍🏻
@aleksuss thanks for merging! I'm super excited about it. :) |
@aleksuss I guess you're testing the tip of master extensively for releasing 0.16.0? FYI, thanks to merging this, we're very close to the plain old version bump to use this facility, which is kind of desired for checking into our master branch: https://github.com/solana-labs/solana/pull/16227/files#r608387625 |
Yeah. I plan to release 0.16.0 soon. |
self.cfs | ||
.cfs | ||
.write() | ||
.unwrap() |
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.
here
Here's the polished version, finally!
#496 (comment)