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

[State Sync] Experiment with using pebble as the execution datastore db #6180

Conversation

UlyanaAndrukhiv
Copy link
Contributor

@UlyanaAndrukhiv UlyanaAndrukhiv commented Jul 4, 2024

Close #6017

Note: this PR includes:

Context

In this pull request:

  • Added a CLI flag (executionDataDBMode) that controls which db type to use.
  • Added DatastoreManager interface that defines the methods for managing a datastore used for handling execution data and badger (BadgerDatastoreManager) and pebble (PebbleDatastoreManager) implementations .
  • Added integration test for pebble execution data .

IPFS PR: ipfs/go-ds-pebble#36

@UlyanaAndrukhiv UlyanaAndrukhiv changed the title Ulyana andrukhiv/6017 pebble as execution datastore db [State Sync] Experiment with using pebble as the execution datastore db Jul 4, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 8.60215% with 170 lines in your changes missing coverage. Please review.

Project coverage is 41.42%. Comparing base (60e14fd) to head (406a64a).

Files Patch % Lines
cmd/access/node_builder/access_node_builder.go 0.00% 38 Missing ⚠️
cmd/observer/node_builder/observer_builder.go 0.00% 37 Missing ⚠️
...cutiondatasync/storage/pebble_datastore_manager.go 0.00% 23 Missing ⚠️
storage/pebble/consumer_progress.go 0.00% 20 Missing ⚠️
...executiondatasync/execution_data/execution_data.go 0.00% 16 Missing ⚠️
...cutiondatasync/storage/badger_datastore_manager.go 0.00% 13 Missing ⚠️
storage/pebble/operation/prefix.go 57.14% 12 Missing ⚠️
storage/pebble/operation/jobs.go 0.00% 6 Missing ⚠️
cmd/execution_builder.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6180      +/-   ##
==========================================
- Coverage   41.49%   41.42%   -0.07%     
==========================================
  Files        2003     2008       +5     
  Lines      142757   142903     +146     
==========================================
- Hits        59232    59196      -36     
- Misses      77352    77532     +180     
- Partials     6173     6175       +2     
Flag Coverage Δ
unittests 41.42% <8.60%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

Added a few comments. I like the direction you're taking, but I think we should separate the tracker updates into a separate PR. That way we could begin to experiment with using pebble as the datastore sooner.

For the tracker changes, I think it would make sense to break it down into 2-3 PRs:

  1. Split out db code similar to how we've done it for all of the other storage interfaces (e.g. storage.Header)
  2. Introduce a new common interface similar to what you have that the badger storage implementation should implement. At this point, there should be no badger specific code left in the tracker/pruner.
  3. Add the pebble version of the storage object.
    I think this will help reduce the coupling of the storage and even simplify the interface

cmd/access/node_builder/access_node_builder.go Outdated Show resolved Hide resolved
cmd/access/node_builder/access_node_builder.go Outdated Show resolved Hide resolved
cmd/access/node_builder/access_node_builder.go Outdated Show resolved Hide resolved
cmd/access/node_builder/access_node_builder.go Outdated Show resolved Hide resolved
cmd/access/node_builder/access_node_builder.go Outdated Show resolved Hide resolved
@UlyanaAndrukhiv UlyanaAndrukhiv marked this pull request as ready for review July 24, 2024 16:33
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

added a few small comments, but I think this is ready to start testing with. we'll need to get the ipfs change merged first (or we can create a fork in the onflow repo if it's going to take a long time)

@peterargue
Copy link
Contributor

I created a fork of ipfs/go-ds-pebble with your changes here: https://github.com/onflow/go-ds-pebble. please update the replace to this repo, then I'll approve

…of github.com:The-K-R-O-K/flow-go into UlyanaAndrukhiv/6017-pebble-as-execution-datastore-db
@UlyanaAndrukhiv
Copy link
Contributor Author

I created a fork of ipfs/go-ds-pebble with your changes here: https://github.com/onflow/go-ds-pebble. please update the replace to this repo, then I'll approve

Thank you, @peter, for creating the forked repo. I have updated the replace.

Copy link
Contributor

@Guitarheroua Guitarheroua left a comment

Choose a reason for hiding this comment

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

Nicely done!

@peterargue peterargue added this pull request to the merge queue Jul 31, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 31, 2024
@peterargue peterargue added this pull request to the merge queue Jul 31, 2024
Merged via the queue into onflow:master with commit d719f5f Jul 31, 2024
55 checks passed
@peterargue peterargue deleted the UlyanaAndrukhiv/6017-pebble-as-execution-datastore-db branch July 31, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[State Sync] Experiment with using pebble as the execution datastore db
4 participants