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

3. refactor(state): move database reads and writes to a new zebra_db module #3579

Merged
merged 6 commits into from
Feb 23, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Feb 18, 2022

Motivation

As part of changing the database, we'll need to make sure all database accesses go through a well-defined API.

As the third step, we need to make sure all database reads and writes go through the same module.
(This makes sure all access to the database respects any consistency rules.)

Solution

Code Movement:

  • split up commit_finalized_direct into validation and writes
  • move database reads and writes to a new zebra_db module
  • add the newly created zebra_db module to the cached state CI job filter

Related Cleanups:

  • make finalized value pool method names consistent

Resolves #2588
(#2588 was needed for #3151 anyway.)

Part of ticket #3151.

Based on PR #3578, feel free to move this PR out of draft after it merges.

Review

@oxarbitrage can review this PR.

git diff --color-moved could make the review a lot easier.

Reviewer Checklist

  • Code moves are correct
  • Existing tests pass

Follow Up Work

  • split up the writes into their own methods, so it's easy to see the changes to each specific type's serialization

@teor2345 teor2345 added C-cleanup Category: This is a cleanup P-Medium ⚡ A-state Area: State / database changes labels Feb 18, 2022
@teor2345 teor2345 self-assigned this Feb 18, 2022
@codecov
Copy link

codecov bot commented Feb 18, 2022

Codecov Report

Merging #3579 (ef8ca0b) into main (fc7ecfe) will decrease coverage by 0.00%.
The diff coverage is 93.11%.

@@            Coverage Diff             @@
##             main    #3579      +/-   ##
==========================================
- Coverage   79.88%   79.87%   -0.01%     
==========================================
  Files         280      281       +1     
  Lines       32554    32582      +28     
==========================================
+ Hits        26005    26025      +20     
- Misses       6549     6557       +8     

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I think this looks good, i will approve after #3578 gets merged.

@teor2345 teor2345 added the A-devops Area: Pipelines, CI/CD and Dockerfiles label Feb 20, 2022
@teor2345 teor2345 force-pushed the rocksdb-low-level-api branch from 6493d05 to 634f5da Compare February 21, 2022 03:00
@teor2345 teor2345 force-pushed the zebra-db-high-level-api branch from c755bd1 to 8a277ae Compare February 21, 2022 03:01
@teor2345 teor2345 force-pushed the rocksdb-low-level-api branch from 634f5da to d3ee9da Compare February 22, 2022 07:04
@teor2345 teor2345 force-pushed the zebra-db-high-level-api branch from 8a277ae to d584e61 Compare February 22, 2022 07:04
@teor2345
Copy link
Contributor Author

I rebased the PR series on main.

Base automatically changed from rocksdb-low-level-api to main February 22, 2022 12:59
@teor2345 teor2345 force-pushed the zebra-db-high-level-api branch from d584e61 to ef8ca0b Compare February 22, 2022 22:23
@teor2345 teor2345 marked this pull request as ready for review February 22, 2022 22:23
@teor2345
Copy link
Contributor Author

@oxarbitrage I rebased this PR on main to resolve conflicts. It should be ready to merge now.

mergify bot added a commit that referenced this pull request Feb 23, 2022
@mergify mergify bot merged commit 22b8a60 into main Feb 23, 2022
@mergify mergify bot deleted the zebra-db-high-level-api branch February 23, 2022 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles A-state Area: State / database changes C-cleanup Category: This is a cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor FinalizedState::commit_finalized_direct
2 participants