fix: remove sleep and refactor code #5107
Conversation
pkg/node/node.go
Outdated
| } | ||
|
|
||
| for _, nc := range closers { | ||
| wg.Add(1) |
There was a problem hiding this comment.
Add to wg before the loop
There was a problem hiding this comment.
Add to wg before the loop
You mean:
wg.Add(len(closers))?
acha-bill
left a comment
acha-bill
left a comment
There was a problem hiding this comment.
Nice. Once small comment
| } | ||
|
|
||
| fmt.Print(beeWelcomeMessage) | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
I was thinking, if the purpose for this sleep is to have beeWelcomeMessage more visible to the users, it would make sense to have this sleep only if the bee is in the terminal. This is just a suggestion, to use term.IsTerminal(int(os.Stdout.Fd())) instead. But only if there is an actual need to have such wait.
There was a problem hiding this comment.
In the spirit of enhancing the node spinup time, I think we can remove it. - even in terminal.
There was a problem hiding this comment.
it's not pretty clear to me what the sleep time has been used for. if its presence affects the node spinup, then sure, remove it. But we need to consider all possible cases it might have an effect on.
There was a problem hiding this comment.
The sleep time was introduced as part of the tribute to ldeffenb , so we can have some seconds to read the quote. I think we can remove now the sleep.
| } | ||
|
|
||
| tryClose(b.ethClientCloser, "eth client") | ||
| tryClose(b.accesscontrolCloser, "accesscontrol") |
There was a problem hiding this comment.
Why did you not include all Closers into namedCloser ?
There was a problem hiding this comment.
With this refactoring, goal was to have the same execution order and reduce the amount of code in same time.
Checklist
Description
Remove time sleep on node start and improve the code in node shutdown.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):