refactor: Split up server functions a bit #16152
Conversation
github-prbot
requested review from
a team,
JeancarloBarrios and
atheeshp
and removed request for
a team
| // Add the tx service to the gRPC router. We only need to register this | ||
| // service if API or gRPC is enabled, and avoid doing so in the general | ||
| // case, because it spawns a new local CometBFT RPC client. | ||
| if svrCfg.API.Enable || svrCfg.GRPC.Enable { |
There was a problem hiding this comment.
Notice that its equivalent to move this here, as tmNode != nil implies its not GRPC only.
| // TODO: Move this to only be done if were launching the node. (So not in GRPC-only mode) | ||
| nodeKey, err := p2p.LoadOrGenNodeKey(cmtCfg.NodeKeyFile()) |
There was a problem hiding this comment.
This should be changed, but would technically change the behavior for Grpc-Only nodes, so would want it to be in its own PR.
server/start.go
Outdated
| // TODO: Why do we reload and unmarshal the entire genesis doc in order to get the chain ID. | ||
| // surely theres a better way. This is likely a serious node start time overhead. | ||
| genDocProvider := getGenDocProvider(cmtCfg) |
There was a problem hiding this comment.
Fixing this (in another PR) would be really helpful!
Maybe should become a follow-up issue?
|
This PR is already really large, so going to stop adding any other fn moves. Let me know if its too big or generally unhelpful I think this can really help us simplify the startup sequence though, as its a frequent source of problem / investigation area |
Personally, I really like smaller and more contained functions. imho decreasing cognitive complexity is def. helpful. |
|
Failing test is code analysis, asking for more test coverage. But I think that should be another PR's role, given that this one is purely refactoring |
Description
Very minor PR, should hopefully be a straightforward review/merge.
The startup server function was 207 lines long, and requires some careful attention to whats going on. I find myself having to look at it semi-occasionally. This PR moves things to sub-functions in a fully compatible manner, and it now takes 109 lines. With minor changes in some follow-ups, this can significantly reduce further.
This PR does some basic splitting up of components of the function, into smaller functions. It surely can be done better, but I essentially did this just to look at it better, so thought this incremental progress would be useful to PR. This area of the code definitely deserves more attention / better refactoring in the future though!
Its fully compatible as far as I can tell. Comments are left in the code to indicate directions for further improvement (and potential existing bugs), but they should be addressed in subsequent PR's.
Let me know if PR's like this aren't helpful.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change