-
Notifications
You must be signed in to change notification settings - Fork 180
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
Expose node component management #6769
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6769 +/- ##
=======================================
Coverage 41.00% 41.01%
=======================================
Files 2103 2103
Lines 184990 184995 +5
=======================================
+ Hits 75862 75873 +11
+ Misses 102764 102762 -2
+ Partials 6364 6360 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
looks good. one small naming suggestion.
Thank you for upgrading the metrics server. I think that should go in a separate PR though
cmd/scaffold.go
Outdated
name: name, | ||
func (fnb *FlowNodeBuilder) Component(name string, f ReadyDoneFactory[*NodeConfig]) NodeBuilder { | ||
fnb.components = append(fnb.components, NamedComponentFunc[*NodeConfig]{ | ||
FN: f, |
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 about calling this initialize or construct?
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.
renamed. Let me know if it is better.
Str("node_role", cfg.BaseConfig.NodeRole). | ||
Hex("spork_id", logging.ID(cfg.SporkID)). |
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.
This change would cause every single line of log to include this two fields, is it necessary?
To me, the node role and spork id does not change after startup, so they only need to be logged once, as long as they can be found easily, we don't have to include them in other logs.
I would suggest to move the role and spork id info back to node startup complete
log.
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.
this logger is used only in this file, which is like 4 log lines. I don't thing the impact is that big.
bacae24
to
8b0dff9
Compare
I wanted to use the framework for composing a node that we have in the gateway without duplicating a lot of the code onflow/flow-evm-gateway#682
The changes I needed were:
FlowNodeImp
has*NodeConfig
which I did not want to use so I pulled out a sub componentNodeImp
that does not have that. This forced me to change some log lines. All the info is still there, but it is in the fields instead of the message.ctx, cancel := context.WithCancel(context.Background())
fromRun
torun
is not strictly necessary, but it was a bit confusing so I refactored it (I can potentially put this in a separate PR)handleComponent
fromFlowNodeBuilder
so I changed it into a function that accepts components and a component builder + the inputAddWorkersFromComponents
. On Flow nodes the input is*NodeConfig
, but with makingReadyDoneFactory
generic the input can be anything.Node.Run
for ease of testing. Closing the context will trigger the shutdown of the node, just like a SIGQUIT