Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove need for &mut self in create_cf and drop_cf (v2) #506

Merged
merged 31 commits into from Apr 6, 2021
Merged

Remove need for &mut self in create_cf and drop_cf (v2) #506

merged 31 commits into from Apr 6, 2021

Conversation

ryoqun
Copy link
Contributor

@ryoqun ryoqun commented Mar 26, 2021

Here's the polished version, finally!

#496 (comment)

@aleksuss Hi, again. I want this to move forward :) (cc: @carllin)

What about to bound an invoke of this with trait ??? I'll try to provide the idea with code soon.

Maybe, were you thinking about like this?: ryoqun@1be386d

Sorry for very rough code. But this eliminates bunch of various concerns arisen from this pr's review:

* ColumnFamily is bounded in the case of MultiThread with zero runtime overhead (I even stopped using Arc)

* ... and no `derive(Clone)` on `ColumnFamily`

* will have no `panic!`s (if everything is correctly rewritten)

* keeps api compatibility and no multithread methods.

* no enum runtime variant branching (although this is cheap)

* downside: a bit of internal code churn (I'll promise I'll work hard to fix them all)

If this direction is really to go from maintainer's perspective, I'll finish this very quickly. :)

@ryoqun ryoqun mentioned this pull request Mar 26, 2021
Closed
ryoqun
ryoqun commented Mar 26, 2021
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
ryoqun
ryoqun commented Mar 26, 2021
View reviewed changes
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();
Copy link
Contributor Author

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?

ryoqun
ryoqun commented Mar 26, 2021
View reviewed changes
src/db.rs Outdated Show resolved Hide resolved
@ryoqun ryoqun changed the title Remove need for &mut self in create_cf and drop_cf Remove need for &mut self in create_cf and drop_cf (v2) Mar 27, 2021
ryoqun
ryoqun commented Mar 27, 2021
View reviewed changes
src/column_family.rs Outdated Show resolved Hide resolved
@ryoqun ryoqun closed this Mar 27, 2021
@ryoqun ryoqun reopened this Mar 27, 2021
@ryoqun
Copy link
Contributor Author

ryoqun commented Mar 27, 2021

@aleksuss Hi, I fixed the CIs too. (latest commits CI aren't run for some reason).

ryoqun
ryoqun commented Mar 29, 2021
View reviewed changes
.github/workflows/rust.yml Outdated Show resolved Hide resolved
ryoqun
ryoqun commented Mar 29, 2021
View reviewed changes
src/db.rs Outdated Show resolved Hide resolved
@aleksuss
Copy link
Member

@ryoqun notify me please when you'll finish to modify the PR 😉

@ryoqun
Copy link
Contributor Author

ryoqun commented Mar 30, 2021 •
edited

@ryoqun notify me please when you'll finish to modify the PR wink

@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. :)

@ryoqun ryoqun mentioned this pull request Mar 30, 2021
Closed
@aleksuss aleksuss mentioned this pull request Apr 1, 2021
Merged
aleksuss
aleksuss reviewed Apr 5, 2021
View reviewed changes
Copy link
Member

@aleksuss aleksuss left a 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.

src/db.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
src/db.rs Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
src/db.rs Show resolved Hide resolved
src/db.rs Show resolved Hide resolved
src/column_family.rs Show resolved Hide resolved
stanislav-tkach
stanislav-tkach approved these changes Apr 5, 2021
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
@ryoqun
Copy link
Contributor Author

ryoqun commented Apr 6, 2021

@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. :)

aleksuss
aleksuss approved these changes Apr 6, 2021
View reviewed changes
Copy link
Member

@aleksuss aleksuss left a comment

Choose a reason for hiding this comment

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

Well done ! 👍🏻

@aleksuss aleksuss merged commit 0b700fe into rust-rocksdb:master Apr 6, 2021
@ryoqun
Copy link
Contributor Author

ryoqun commented Apr 6, 2021

@aleksuss thanks for merging! I'm super excited about it. :)

@ryoqun
Copy link
Contributor Author

ryoqun commented Apr 7, 2021

@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

@aleksuss
Copy link
Member

aleksuss commented Apr 7, 2021

Yeah. I plan to release 0.16.0 soon.

@ryoqun
Copy link
Contributor Author

ryoqun commented Apr 13, 2021

aleksuss merged commit 0b700fe into rust-rocksdb:master 7 days ago

Yeah. I plan to release 0.16.0 soon.

🐶

@aleksuss I can lend any hand to make this happen soon. Alternatively, could you share any ETAs for this, not to make your lovely contributing puppy (= me, lol)'s eyes dim? :)

@carllin carllin mentioned this pull request Apr 16, 2021
Closed
@fanatid fanatid mentioned this pull request Apr 17, 2021
Merged
@ryoqun ryoqun mentioned this pull request Jul 5, 2021
Merged
ryoqun
ryoqun commented Jul 21, 2021
View reviewed changes
src/db.rs
self.cfs
.cfs
.write()
.unwrap()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stanislav-tkach stanislav-tkach stanislav-tkach approved these changes

@aleksuss aleksuss aleksuss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ryoqun @aleksuss @stanislav-tkach @carllin @CriesofCarrots