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

6. refactor(state): prepare finalized state for shared read-only access #3810

Merged
merged 7 commits into from
Mar 11, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Mar 9, 2022

Motivation

As part of #3745, we need to be able to clone read-only access to the finalized state.

Designs

The DiskDb type provides shared read-write access to the low-level types in the finalized state. (Like individual column family entries.)

That's enough for initial performance testing, but later PRs will allow shared access to high-level types. (Like composite types and queries.)

Write access requires a lot of manual low-level code, so it should be obvious in reviews. The high-level type wrapper might also prevent write access, but that's not a priority.

Solution

  • wrap the underlying RocksDB instance in an Arc
  • only shut down the database when the last shared instance is dropped
  • implement Eq

Review

This is a bit urgent, because it blocks most of the lightwalletd integration tests.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

@teor2345 teor2345 added C-enhancement Category: This is an improvement P-Medium ⚡ I-slow Problems with performance or responsiveness A-state Area: State / database changes labels Mar 9, 2022
@teor2345 teor2345 requested a review from a team as a code owner March 9, 2022 07:29
@teor2345 teor2345 self-assigned this Mar 9, 2022
@teor2345 teor2345 requested review from dconnolly and removed request for a team March 9, 2022 07:29
@teor2345 teor2345 changed the title refactor(state): prepare finalized state for shared read-only access 6. refactor(state): prepare finalized state for shared read-only access Mar 9, 2022
@teor2345
Copy link
Contributor Author

teor2345 commented Mar 9, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Mar 9, 2022

update

☑️ Nothing to do

  • -closed [:pushpin: update requirement]
  • #commits-behind>0 [:pushpin: update requirement]

@teor2345 teor2345 requested a review from a team as a code owner March 9, 2022 08:08
@codecov
Copy link

codecov bot commented Mar 9, 2022

Codecov Report

Merging #3810 (d8a79b6) into split-state-service (984e0b1) will decrease coverage by 0.03%.
The diff coverage is 59.64%.

@@                   Coverage Diff                   @@
##           split-state-service    #3810      +/-   ##
=======================================================
- Coverage                78.86%   78.82%   -0.04%     
=======================================================
  Files                      293      293              
  Lines                    33560    33603      +43     
=======================================================
+ Hits                     26466    26487      +21     
- Misses                    7094     7116      +22     

Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks good, just have a question. Feel free to mark it as solved if there is no issue there.

zebra-state/src/service/finalized_state/disk_db.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 requested review from conradoplg and removed request for dconnolly March 9, 2022 20:26
@teor2345
Copy link
Contributor Author

teor2345 commented Mar 9, 2022

Fetching the Zcash parameters caused the Windows test to time out.

conradoplg
conradoplg previously approved these changes Mar 9, 2022
@teor2345 teor2345 force-pushed the split-state-service branch from a8cbc09 to 984e0b1 Compare March 10, 2022 01:34
@teor2345 teor2345 requested a review from a team as a code owner March 10, 2022 01:34
@teor2345 teor2345 force-pushed the prep-read-finalized-state branch from 2dd2f9b to d8a79b6 Compare March 10, 2022 02:25
@teor2345 teor2345 removed the request for review from a team March 10, 2022 02:26
Base automatically changed from split-state-service to main March 10, 2022 20:40
@teor2345 teor2345 requested a review from conradoplg March 10, 2022 20:53
mergify bot added a commit that referenced this pull request Mar 10, 2022
@mergify mergify bot merged commit 199267b into main Mar 11, 2022
@mergify mergify bot deleted the prep-read-finalized-state branch March 11, 2022 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-state Area: State / database changes C-enhancement Category: This is an improvement I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants