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
statesync/event: emit statesync start/end event #6700
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6700 +/- ##
==========================================
+ Coverage 62.31% 62.48% +0.17%
==========================================
Files 297 298 +1
Lines 39907 39962 +55
==========================================
+ Hits 24867 24972 +105
+ Misses 13280 13223 -57
- Partials 1760 1767 +7
|
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.
Left some preliminary comments. Great work @JayT106.
node/node.go
Outdated
if err != nil { | ||
ssR.Logger.Error("state sync failed", "err", err) | ||
ssR.GetLogger().Error("state sync failed", "err", err) |
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.
codesmell. Don't log here. The reactor should log on error.
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.
but it is a different goroutine, the caller might not be able to receive the return error.
@alexanderbez thanks for the feedback, I will change/remove the logs. |
48b78c8
to
d3f61ad
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.
Great changes so far @JayT106. Almost there!
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.
Just a few syntax changes but otherwise good
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 - Thanks!
0655ea0
to
019e081
Compare
@JayT106 - when you get a moment do you mind resolving the conflicts here. Then we can get this merged |
d728550
to
3370cd0
Compare
#6619 (comment)