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

refactor(daser): use functional options pattern to configure daser #1225

Merged
merged 4 commits into from
Oct 28, 2022

Conversation

derrandz
Copy link
Contributor

@derrandz derrandz commented Oct 12, 2022

Overview

The DAS module is currently reliant on default constant global variables acting as configuration variables.

This makes changing configuration values for the DAS module non-trivial, as it requires code changes and binary re-building.

In this PR, we refactor the global configuration variables used in the DAS module to use the functional options pattern, which will trivialize the process of configuring a Celestia node.

Reference Issue

#1063
#709

Impact / Change

The das package is now configurable using a set of functional options, namely:

WithSamplingRange(uint64)
WithConcurrencyLimit(int)
WithBackgroundStoreInterval(time.Duration)
WithPrioritySizeQueue(int)
WithGenesisHeight)

This allows the das package to be configured with values other than the default parameters defined in das/options.go:29

An example of the das package being configured with these options and instantiated with them is: nodebuilder/das/module.go:22

Owners

@derrandz

das/daser.go Outdated Show resolved Hide resolved
nodebuilder/config.go Show resolved Hide resolved
das/coordinator_test.go Outdated Show resolved Hide resolved
das/coordinator_test.go Outdated Show resolved Hide resolved
Copy link
Member

@walldiss walldiss 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 to me. Like your functional approach with options to remove dependancy on nodebuilder. There are 2 things that I think could be improved:

  • types in structs could be better aligned with their usage to avoid extra conversions.
  • we need to set up default values inside DASer in case missing options scenario.

Besides that looks legit.

nodebuilder/daser/config.go Outdated Show resolved Hide resolved
nodebuilder/daser/config.go Outdated Show resolved Hide resolved
das/daser.go Outdated Show resolved Hide resolved
das/state.go Outdated Show resolved Hide resolved
das/state.go Outdated Show resolved Hide resolved
das/daser.go Outdated Show resolved Hide resolved
das/coordinator_test.go Outdated Show resolved Hide resolved
das/coordinator.go Outdated Show resolved Hide resolved
@derrandz derrandz force-pushed the refactor/daser-options-pattern branch from 1579caf to 9ac8dbd Compare October 18, 2022 20:33
@derrandz
Copy link
Contributor Author

derrandz commented Oct 18, 2022

@walldiss I've addressed your comments about typing to do away with conversions in operations, however, I would still like to understand why are the compared-with variables uint64s to begin with? variables like networkHead and so on.

das/daser.go Outdated Show resolved Hide resolved
das/daser.go Outdated Show resolved Hide resolved
das/daser.go Outdated Show resolved Hide resolved
nodebuilder/daser/config.go Outdated Show resolved Hide resolved
nodebuilder/daser/config.go Outdated Show resolved Hide resolved
das/daser.go Outdated Show resolved Hide resolved
das/coordinator_test.go Outdated Show resolved Hide resolved
das/coordinator_test.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2022

Codecov Report

Merging #1225 (6793a3f) into main (f4e582e) will decrease coverage by 0.66%.
The diff coverage is 70.63%.

@@            Coverage Diff             @@
##             main    #1225      +/-   ##
==========================================
- Coverage   56.34%   55.68%   -0.67%     
==========================================
  Files         160      164       +4     
  Lines        9935    10079     +144     
==========================================
+ Hits         5598     5612      +14     
- Misses       3783     3906     +123     
- Partials      554      561       +7     
Impacted Files Coverage Δ
nodebuilder/das/opts.go 0.00% <ø> (ø)
nodebuilder/module.go 100.00% <ø> (ø)
nodebuilder/settings.go 0.00% <0.00%> (ø)
das/options.go 53.96% <53.96%> (ø)
nodebuilder/das/config.go 70.00% <70.00%> (ø)
das/daser.go 65.21% <75.00%> (-1.07%) ⬇️
das/state.go 95.51% <94.11%> (+0.05%) ⬆️
das/coordinator.go 92.64% <100.00%> (ø)
nodebuilder/config.go 86.11% <100.00%> (+0.39%) ⬆️
nodebuilder/das/daser.go 100.00% <100.00%> (ø)
... and 31 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

das/options.go Outdated Show resolved Hide resolved
das/daser.go Outdated Show resolved Hide resolved
das/daser_test.go Outdated Show resolved Hide resolved
das/options.go Outdated Show resolved Hide resolved
nodebuilder/das/config.go Outdated Show resolved Hide resolved
das/options.go Outdated Show resolved Hide resolved
@renaynay
Copy link
Member

Also @derrandz can you check all your godocs on the methods you've implemented and make it according to godoc style?

for example:

// Type Parameters .....
type Parameters struct {

should be

// Parameters .....
type Parameters struct {

@derrandz derrandz force-pushed the refactor/daser-options-pattern branch from 61103ed to 5fd623b Compare October 27, 2022 09:57
das/options.go Outdated Show resolved Hide resolved
das/state.go Outdated Show resolved Hide resolved
das/state.go Outdated Show resolved Hide resolved
nodebuilder/das/config.go Show resolved Hide resolved
nodebuilder/das/daser.go Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Oct 27, 2022
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this over the finish line. This PR was a great way for us to develop the configuration pattern for our codebase and get everyone onboarded with it.

nodebuilder/das/config.go Outdated Show resolved Hide resolved
das/daser_test.go Outdated Show resolved Hide resolved
nodebuilder/das/config.go Outdated Show resolved Hide resolved
nodebuilder/das/config.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Oct 27, 2022
renaynay
renaynay previously approved these changes Oct 28, 2022
walldiss
walldiss previously approved these changes Oct 28, 2022
Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

Looks amazing, Great work!

@derrandz derrandz dismissed stale reviews from walldiss, renaynay, and Wondertan via 9c79cd4 October 28, 2022 09:31
@derrandz derrandz force-pushed the refactor/daser-options-pattern branch 2 times, most recently from 9c79cd4 to daa6840 Compare October 28, 2022 09:34
@derrandz derrandz merged commit fb17d24 into main Oct 28, 2022
@derrandz derrandz deleted the refactor/daser-options-pattern branch October 28, 2022 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config CLI and config area:das Related to DASer kind:feat Attached to feature PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

das: make DAS'er params to be configurable
5 participants