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

change(zebrad): opens the database in a blocking tokio thread, which allows tokio to run other tasks #5228

Merged
merged 4 commits into from
Sep 26, 2022
Merged

change(zebrad): opens the database in a blocking tokio thread, which allows tokio to run other tasks #5228

merged 4 commits into from
Sep 26, 2022

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Sep 21, 2022 •
edited by teor2345
Loading

Motivation

To allow tokio to run other tasks while the database is opening and to tell users that it may take a couple minutes

Closes #4821.

Solution

  • Move the initialization of zebra-state and the database to a blocking thread
  • Log an info-level message before starting to open the database

Review

This PR is part of regular scheduled work.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
  • How do you know it works? Does it have tests?

Follow Up Work

  • Do the first read in a blocking tokio thread. (Some RocksDB documentation warns that that can also be slow)

@arya2 arya2 added A-state Area: State / database changes C-bug Category: This is a bug P-Low ❄️ I-slow Problems with performance or responsiveness I-usability Zebra is hard to understand or use labels Sep 21, 2022
@arya2 arya2 marked this pull request as ready for review September 21, 2022 22:56
@arya2 arya2 requested a review from a team as a code owner September 21, 2022 22:56
@arya2 arya2 requested review from dconnolly and removed request for a team September 21, 2022 22:56
@arya2 arya2 changed the title change(zebrad) allows tokio to run other tasks while zebra opens the database with a blocking thread change(zebrad) opens the database with a blocking thread, which allows tokio to run other tasks Sep 21, 2022
@arya2 arya2 changed the title change(zebrad) opens the database with a blocking thread, which allows tokio to run other tasks change(zebrad) opens the database in a blocking tokio thread, which allows tokio to run other tasks Sep 21, 2022
@arya2 arya2 force-pushed the open-db-in-blocking-thread branch from bd3bb89 to 84be954 Compare September 21, 2022 23:12
@codecov
Copy link

codecov bot commented Sep 22, 2022 •
edited
Loading

Codecov Report

Merging #5228 (111777e) into main (9cb6c55) will increase coverage by 0.08%.
The diff coverage is 73.33%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5228      +/-   ##
==========================================
+ Coverage   79.13%   79.21%   +0.08%     
==========================================
  Files         307      307              
  Lines       39225    39229       +4     
==========================================
+ Hits        31039    31074      +35     
+ Misses       8186     8155      -31

@teor2345 teor2345 changed the title change(zebrad) opens the database in a blocking tokio thread, which allows tokio to run other tasks change(zebrad): opens the database in a blocking tokio thread, which allows tokio to run other tasks Sep 22, 2022
teor2345
teor2345 approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I like how you designed the new API, and the tests!

mergify bot added a commit that referenced this pull request Sep 22, 2022
@mergify
Merge of #5228
e4d7980
@mergify mergify bot mentioned this pull request Sep 22, 2022
Closed
mergify bot added a commit that referenced this pull request Sep 22, 2022
@mergify
Merge of #5228
d3ad53b
@mergify mergify bot mentioned this pull request Sep 22, 2022
Closed
@teor2345
Copy link
Contributor

@arya2 looks like this failed in the Mergify PR, did you need help working out why?

@arya2
Copy link
Contributor Author

arya2 commented Sep 22, 2022

@arya2 looks like this failed in the Mergify PR, did you need help working out why?

Yes please.

@teor2345
Copy link
Contributor

@arya2 looks like this failed in the Mergify PR, did you need help working out why?

Yes please.

I just realised we didn't have this documented anywhere, so I wrote something up:
https://github.com/ZcashFoundation/zebra/pull/5240/files

In summary, we go to the Mergify PR and find the errors on that PR, just like we would any other PR. It looks like this one failed downloading and caching the Zcash parameters. That's a transient network error, so we can just retry Mergify.

@teor2345
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2022

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Sep 22, 2022
@mergify
Merge of #5228
4d614a2
@mergify mergify bot mentioned this pull request Sep 22, 2022
Closed
@arya2
Copy link
Contributor Author

arya2 commented Sep 23, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2022

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Sep 23, 2022
@mergify
Merge of #5228
300f3db
@mergify mergify bot mentioned this pull request Sep 23, 2022
Closed
@teor2345
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2022

refresh

✅ Pull request refreshed

@teor2345
Copy link
Contributor

@arya2 usually if an error happens twice, it's not a transient network error, and we have to look for other causes.

In this case, it seems to be an issue with the new Rust release and our old compiled code caches. I added that to the instructions in #5240

mergify bot added a commit that referenced this pull request Sep 23, 2022
@mergify
Merge of #5228
58035be
@mergify mergify bot mentioned this pull request Sep 23, 2022
Closed
mergify bot added a commit that referenced this pull request Sep 23, 2022
@mergify
Merge of #5228
fe3c12c
@mergify mergify bot mentioned this pull request Sep 23, 2022
Closed
@arya2
Copy link
Contributor Author

arya2 commented Sep 23, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2022

refresh

✅ Pull request refreshed

@arya2
Copy link
Contributor Author

arya2 commented Sep 23, 2022 •
edited
Loading

download-params is failing on main too. There's currently no actions cache for this PR, I'll try clearing all of the caches if it fails again.

mergify bot added a commit that referenced this pull request Sep 23, 2022
@mergify
Merge of #5228
d615eea
@mergify mergify bot mentioned this pull request Sep 23, 2022
Closed
@arya2
Copy link
Contributor Author

arya2 commented Sep 23, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2022

update

✅ Branch has been successfully updated

@teor2345
Copy link
Contributor

It looks like this is actually a Rust 1.64 issue, see #5251

@upbqdn
Copy link
Member

upbqdn commented Sep 26, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Sep 26, 2022

update

✅ Branch has been successfully updated

@mergify mergify bot merged commit ec115e9 into main Sep 26, 2022
@mergify mergify bot deleted the open-db-in-blocking-thread branch September 26, 2022 15:45
@teor2345 teor2345 mentioned this pull request Sep 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

@dconnolly dconnolly Awaiting requested review from dconnolly dconnolly is a code owner automatically assigned from ZcashFoundation/general-rust-reviewers

Assignees

@arya2 arya2

Labels
A-state Area: State / database changes C-bug Category: This is a bug I-slow Problems with performance or responsiveness I-usability Zebra is hard to understand or use
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Open the database in a blocking tokio thread
3 participants
@arya2 @teor2345 @upbqdn