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

Vishal/consensus follower storage #1228

Merged
merged 3 commits into from
Sep 3, 2021
Merged

Vishal/consensus follower storage #1228

merged 3 commits into from
Sep 3, 2021

Conversation

vishalchangrani
Copy link
Contributor

From @awfm9

We have another problem: we need to access the protocol state database in our code. However, since the DB is not injected into the consensus follower as a dependency, but rather you just pass the data directory as an option, the DB is opened internally by the follower's node builder.
That makes it impossible for us to access the protocol state database, as it's not exposed in any meaningful way. Would it be possible to change that so that the node builder takes a database handle, and the consensus follower takes a *badger.DB as well, rather than a data directory?

This change allows the Badger db to be passed in as a dependency.

huitseeker
huitseeker previously approved these changes Aug 31, 2021
View reviewed changes
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Thanks!

cmd/scaffold.go
@@ -717,6 +724,12 @@ func WithMetricsEnabled(enabled bool) Option {
}
}

func WithDB(db *badger.DB) Option {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way to integrate this somewhere along our existing integration tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes- been working on the side on this unittest - https://github.com/onflow/flow-go/blob/vishal/consensus_follower_unittest/follower/consensus_follower_test.go#L108.

This uses this new WithDB call. Also, the unit test should make testing way more easier than the current integration test.

@huitseeker huitseeker dismissed their stale review August 31, 2021 20:19

We should merge identity-provider first

@synzhu
Copy link
Contributor

synzhu commented Sep 1, 2021

@vishalchangrani difficult to review this until identity provider is merged

@huitseeker huitseeker force-pushed the smnzhu/identity-provider branch 2 times, most recently from 60b5c1d to bf97722 Compare September 2, 2021 14:44
@vishalchangrani
Copy link
Contributor Author

@vishalchangrani difficult to review this until identity provider is merged

identity provider has been merged in to reflect only the things that have changed on this PR.

m4ksio
m4ksio approved these changes Sep 2, 2021
View reviewed changes
cmd/scaffold.go
fnb.DB = fnb.BaseConfig.db
return
}

// Pre-create DB path (Badger creates only one-level dirs)
err := os.MkdirAll(fnb.BaseConfig.datadir, 0700)
fnb.MustNot(err).Str("dir", fnb.BaseConfig.datadir).Msg("could not create datadir")
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good if we reject the datadir flag when DB is passed in. Ie. make sure its not set, to avoid dangling unused configs and potential confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made that change - to ignore datadir if db is passed in.

vishalchangrani
vishalchangrani commented Sep 2, 2021
View reviewed changes
follower/consensus_follower.go
func WithDataDir(dataDir string) Option {
return func(cf *Config) {
cf.dataDir = dataDir
if cf.db == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m4ksio - modified to ignore the dataDir if db is passed in

Base automatically changed from smnzhu/identity-provider to master September 2, 2021 18:53
@huitseeker huitseeker force-pushed the vishal/consensus_follower_storage branch from 8778155 to dc8cab2 Compare September 2, 2021 19:20
@codecov-commenter
Copy link

Codecov Report

Merging #1228 (dc8cab2) into master (4b25ece) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1228      +/-   ##
==========================================
+ Coverage   56.20%   56.23%   +0.02%     
==========================================
  Files         495      495              
  Lines       30124    30124              
==========================================
+ Hits        16932    16940       +8     
+ Misses      10893    10885       -8     
  Partials     2299     2299
Flag Coverage Δ
unittests 56.23% <ø> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
module/mempool/epochs/transactions.go 100.00% <0.00%> (+5.26%) ⬆️
...sus/approvals/assignment_collector_statemachine.go 47.11% <0.00%> (+6.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b25ece...dc8cab2. Read the comment docs.

@awfm9
Copy link
Contributor

awfm9 commented Sep 3, 2021

We need this to get merged in order to finalize integration of the unstaked consensus follower into the DPS.

huitseeker
huitseeker approved these changes Sep 3, 2021
View reviewed changes
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

👍

@huitseeker
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Sep 3, 2021

Build succeeded:

@bors bors bot merged commit 24a5f3a into master Sep 3, 2021
@bors bors bot deleted the vishal/consensus_follower_storage branch September 3, 2021 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huitseeker huitseeker huitseeker approved these changes

@m4ksio m4ksio m4ksio approved these changes

@synzhu synzhu synzhu approved these changes

@AlexHentschel AlexHentschel Awaiting requested review from AlexHentschel

@jordanschalm jordanschalm Awaiting requested review from jordanschalm

@ramtinms ramtinms Awaiting requested review from ramtinms

@yhassanzadeh13 yhassanzadeh13 Awaiting requested review from yhassanzadeh13

@zhangchiqing zhangchiqing Awaiting requested review from zhangchiqing

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@vishalchangrani @synzhu @codecov-commenter @awfm9 @huitseeker @m4ksio