Remove need for &mut self in create_cf and drop_cf (v2) by ryoqun · Pull Request #506 · rust-rocksdb/rust-rocksdb

ryoqun · 2021-03-26T16:18:22Z

Here's the polished version, finally!

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

src/lib.rs

ryoqun · 2021-03-26T16:27:20Z

tests/test_db.rs

+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();


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?

src/db.rs

src/column_family.rs

ryoqun · 2021-03-27T09:42:22Z

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

.github/workflows/rust.yml

src/db.rs

aleksuss · 2021-03-30T14:03:29Z

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

ryoqun · 2021-03-30T15:04:08Z

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

aleksuss

Overall looks pretty good as 61f323b.
But couple nitpicks.

src/db.rs

src/column_family.rs

Cargo.toml

src/db.rs

ryoqun · 2021-04-06T11:48:38Z

@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

Well done ! 👍🏻

ryoqun · 2021-04-06T14:23:56Z

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

ryoqun · 2021-04-07T07:01:52Z

@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 · 2021-04-07T07:29:06Z

Yeah. I plan to release 0.16.0 soon.

ryoqun · 2021-04-13T03:00:15Z

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? :)

ryoqun · 2021-07-21T16:11:37Z

src/db.rs

+        self.cfs
+            .cfs
+            .write()
+            .unwrap()


carllin and others added 15 commits February 25, 2021 23:23

Use RwLock

f6eafe1

Update create_cf

123d613

Test fixup

0ead892

Add multithread column family map option

c51484c

Add comments/cleanup

0f5a3cf

Plumb is_multithreaded through constructors

e88da39

Fix tests

c1a203e

Add multithreaded test for column families

1fc975f

Remove clone of handle

728b349

Rework test

ac09e75

Revert references

441834e

Add comment

a6b5366

Merge branch 'master' into DropCf

9f0fb18

Encode single/multi threading into types

1be386d

Finish off implementing and cleaning

78aeab5

ryoqun mentioned this pull request Mar 26, 2021

Remove need for &mut self in create_cf and drop_cf #496

Closed

ryoqun commented Mar 26, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

ryoqun commented Mar 26, 2021

View reviewed changes

src/db.rs Outdated Show resolved Hide resolved

Fix ci

6c9fce8

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 added 3 commits March 27, 2021 17:12

Further fix ci

2ce61ec

Further tweak ci actions

9999360

Fix typo

82e57d1

ryoqun commented Mar 27, 2021

View reviewed changes

src/column_family.rs Outdated Show resolved Hide resolved

ryoqun closed this Mar 27, 2021

ryoqun reopened this Mar 27, 2021

Update rust.yml

42bdf42

aleksuss requested a review from stanislav-tkach March 29, 2021 08:20

ryoqun commented Mar 29, 2021

View reviewed changes

.github/workflows/rust.yml Outdated Show resolved Hide resolved

ryoqun commented Mar 29, 2021

View reviewed changes

src/db.rs Outdated Show resolved Hide resolved

ryoqun added 3 commits March 30, 2021 20:18

Impl Send for BoundCF & add type alias for compat

67a180a

Replace all remaining &CF with impl CFRef

c722e16

Expose compile-time handy ColumnFamilyRef alias

4016943

ryoqun added 2 commits March 30, 2021 23:18

Tweaks

d687029

Remove unneeded cfgs...

61f323b

ryoqun mentioned this pull request Mar 30, 2021

Multi threaded rocksdb cf solana-labs/solana#16227

Closed

aleksuss mentioned this pull request Apr 1, 2021

librocksdb-sys: bump to 6.17.3 #503

Merged

aleksuss reviewed Apr 5, 2021

View reviewed changes

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 added 6 commits April 6, 2021 19:21

Rename InternalDbAdapter => DbAccess

7a16581

Rename: map => cfs

f8c7fd4

Rename: multi-threaded-as-default => multi-threaded-cf-alteration

a590597

Remove unsafe from trait definition

1a11b5f

Improve docs

2cfe746

Test error by creating CFs via SingleThreaded &ref

9fd26a3

aleksuss approved these changes Apr 6, 2021

View reviewed changes

aleksuss merged commit 0b700fe into rust-rocksdb:master Apr 6, 2021

carllin mentioned this pull request Apr 16, 2021

Dropping rocks column families instead of running compaction solana-labs/solana#16589

Closed

fanatid mentioned this pull request Apr 17, 2021

Update dependencies to support Apple M1, fix Windows CI LLVM paths ZcashFoundation/zebra#2026

Merged

ryoqun mentioned this pull request Jul 5, 2021

Don't leak dropped column families #509

Merged

ryoqun commented Jul 21, 2021

View reviewed changes

src/db.rs

self.cfs

.cfs

.write()

.unwrap()

Copy link
Copy Markdown

Contributor Author

ryoqun Jul 21, 2021

Choose a reason for hiding this comment

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

here

Conversation

ryoqun commented Mar 26, 2021

Uh oh!

Uh oh!

ryoqun Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryoqun commented Mar 27, 2021

Uh oh!

Uh oh!

Uh oh!

aleksuss commented Mar 30, 2021

Uh oh!

ryoqun commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aleksuss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryoqun commented Apr 6, 2021

Uh oh!

aleksuss left a comment

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Apr 6, 2021

Uh oh!

ryoqun commented Apr 7, 2021

Uh oh!

aleksuss commented Apr 7, 2021

Uh oh!

ryoqun commented Apr 13, 2021

Uh oh!

ryoqun Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryoqun commented Mar 30, 2021 •

edited

Loading