-
Notifications
You must be signed in to change notification settings - Fork 187
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
[v2] Controller Entrypoint #873
Conversation
To be rebased on top of #871 |
e6a2d4f
to
426c3cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm so far, left a couple comments
Name: common.PrefixFlag(FlagPrefix, "available-relays"), | ||
Usage: "List of available relays", | ||
Required: true, | ||
EnvVar: common.PrefixEnvVar(envVarPrefix, "AVAILABLE_RELAYS"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work? Each relay has a integer assigned to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this will be a list of integer representing the disperser's relay keys
} | ||
NodeClientCacheSizeFlag = cli.IntFlag{ | ||
Name: common.PrefixFlag(FlagPrefix, "node-client-cache-size"), | ||
Usage: "Size of the node client cache", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the unit of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Number of entries. Updated the name accordingly
a8bc74f
to
9f2a652
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will approve after rebase.
dispatcherPool := workerpool.New(config.NumConcurrentDispersalRequests) | ||
chainState := eth.NewChainState(chainReader, gethClient) | ||
var ics core.IndexedChainState | ||
if config.UseGraph { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the intention behind retaining the built-in indexer? Is this for testing purposes? Can we safely remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is to support testing mode that doesn't use graph indexer.
I kept it because non-graph e2e tests have been useful few times in the past
9f2a652
to
540efe4
Compare
540efe4
to
d4f2faa
Compare
Why are these changes needed?
Adds an entrypoint to controller that kicks off encoding manager and dispatcher.
TODO: dependency to encoding client to be resolved in follow up PR when #866 is merged
Checks