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.

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

Make Db::new async, force use of new API #323

Merged
merged 19 commits into from
Oct 26, 2023
Merged

Conversation

rkuris
Copy link
Collaborator

@rkuris rkuris commented Oct 17, 2023

This somewhat-large diff pushes some of the async down the stack a little. It's not complete but it's a good stop point for the next push down.

This somewhat-large diff pushes some of the async down the stack a
little. It's not complete but it's a good stop point for the next push
down.
Copy link
Contributor

@richardpringle richardpringle left a comment

Choose a reason for hiding this comment

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

I know you've already pushed changes, but I figure I will leave this review here now anyway

firewood/benches/hashops.rs Outdated Show resolved Hide resolved
firewood/src/db.rs Outdated Show resolved Hide resolved
firewood/src/db.rs Outdated Show resolved Hide resolved
firewood/src/file.rs Show resolved Hide resolved
firewood/src/storage/buffer.rs Outdated Show resolved Hide resolved
firewood/tests/db.rs Show resolved Hide resolved
firewood/tests/db.rs Show resolved Hide resolved
firewood/tests/db.rs Show resolved Hide resolved
firewood/tests/db.rs Show resolved Hide resolved
firewood/tests/v2api.rs Show resolved Hide resolved
This eliminates a lot of complexity, but callers must still import the
api::* in order to use the db object.
@rkuris rkuris marked this pull request as draft October 19, 2023 15:03
This leaves metrics not working; we need a reference to the Db object
from a DbRev in order for that to be fixed

Calling revision(hash) on the root hash after creating an empty database
fails. This can and should be fixed.
These need to happen whenever IO happens, these few spots were missed
@rkuris rkuris self-assigned this Oct 24, 2023
@rkuris rkuris marked this pull request as ready for review October 24, 2023 18:19
firewood/benches/hashops.rs Outdated Show resolved Hide resolved
firewood/examples/insert.rs Show resolved Hide resolved
firewood/src/db.rs Show resolved Hide resolved
firewood/src/file.rs Show resolved Hide resolved
firewood/src/lib.rs Show resolved Hide resolved
firewood/tests/db.rs Outdated Show resolved Hide resolved
firewood/tests/db.rs Outdated Show resolved Hide resolved
firewood/tests/db.rs Outdated Show resolved Hide resolved
firewood/src/db.rs Outdated Show resolved Hide resolved
firewood/src/db.rs Outdated Show resolved Hide resolved
 - Fix extra async during benchmarks
 - Make sure we always use CARGO_TARGET_DIR (not CARGO_TARGET_TMPDIR)
   (the latter only works during integration tests)
 - Add back concurrent insert tests (even though they probably will
   always work)
rkuris and others added 3 commits October 25, 2023 22:20
Co-authored-by: Richard Pringle <richard.pringle@avalabs.org>
Signed-off-by: Ron Kuris <swcafe@gmail.com>
Copy link
Contributor

@richardpringle richardpringle left a comment

Choose a reason for hiding this comment

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

I'm going to approve this, but I really don't think the changes in firewood/src/storage/buffer.rs should be checked in. Tests should only be made async on an as-needed basis.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these changes should be committed, there's no benefit to them. We should only make tests async that actually need to be async. We can do that on an as-needed basis.

I really don't think this code should be merged, to be honest. There's no reason for it. You can just stash these changes locally and re-apply them when you need them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that a future diff will make these functions wait for I/O and will need to support async. I'm just getting an early start.

@@ -172,7 +174,7 @@ pub trait DbView {
/// [DbView], which means you can fetch values from it or
/// obtain proofs.
#[async_trait]
pub trait Proposal: DbView {
pub trait Proposal: DbView + Send + Sync {
Copy link
Contributor

@richardpringle richardpringle Oct 26, 2023

Choose a reason for hiding this comment

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

firewood git:(main) gh pr checkout 323
Switched to branch 'rkuris/async-down-stack'
Your branch is up to date with 'origin/rkuris/async-down-stack'.
Already up to date.firewood git:(rkuris/async-down-stack) sd '(DbView) \+ Send \+ Sync' '$1' firewood/src/v2/api.rsfirewood git:(rkuris/async-down-stack) ✗ git --no-pager diff
diff --git a/firewood/src/v2/api.rs b/firewood/src/v2/api.rs
index 0ddacbe..dbc9ff4 100644
--- a/firewood/src/v2/api.rs
+++ b/firewood/src/v2/api.rs
@@ -172,7 +172,7 @@ pub trait DbView {
 /// [DbView], which means you can fetch values from it or
 /// obtain proofs.
 #[async_trait]
-pub trait Proposal: DbView + Send + Sync {
+pub trait Proposal: DbView {
     type Proposal: DbView + Proposal;

     /// Commit this revisionfirewood git:(rkuris/async-down-stack) ✗ cargo check -p firewood
    Checking firewood v0.0.4 (/<omitted>/firewood/firewood)
    Finished dev [unoptimized + debuginfo] target(s) in 0.40sfirewood git:(rkuris/async-down-stack) ✗

FYI
This might not be the case for you, but I never remember how to use sed, I much prefer sd.

ripgrep is also much better than grep 😉

});
let (first, second) = tokio::join!(t1, t2);
first.unwrap();
second.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@richardpringle
Copy link
Contributor

I'm going to approve this, but I really don't think the changes in firewood/src/storage/buffer.rs should be checked in. Tests should only be made async on an as-needed basis.

I swear I hit the approve button... weird. But Send + Sync needs to be removed, so this is fine like this. Would love to discuss the buffer.rs changes, but they shouldn't block the PR

Signed-off-by: Ron Kuris <ron.kuris@avalabs.org>
@rkuris rkuris merged commit 20d6110 into main Oct 26, 2023
5 checks passed
@rkuris rkuris deleted the rkuris/async-down-stack branch October 26, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants