-
-
Notifications
You must be signed in to change notification settings - Fork 290
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
fix: move and fix execution engine state update checks for online state #5862
Conversation
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.
Do we really want to print out the enum values in all caps? oldState=SYNCED, newState=ONLINE
newState is already part of the message and oldState should be logged previously. There are also not that much changes of state where this would be helpful, most of the time it is from syncing to synced.
@g11tech That change will make the tests fail, will fix those and update the PR. |
@nflaig The message is more descriptive, and having extra information and context does not harm, rather useful for log parsing for someone. |
Performance Report✔️ no performance regression detected Full benchmark results
|
@nazarhussain maybe just add previous state then? The log message already contains the newState hence it is duplicated information. Maybe not all caps would be preferable as well One other note, on startup we print this
This is confusing to me, does this mean the EL was not synced before? I know it doesn't but will the user know? Before it just sayed |
i think the current logging is informative as well as easily understood , we always print out enum in caps i think so i guess that is fine as well |
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 to me, @nazarhussain you may approve the PR and merge (I as author can't do the same)
Noticed that PR - fix: offline error message when node is shutting down #5797 was updating execution state from SYNCED to ONLINE since the checks for the same were not located at correct place which caused excessive logging: here is an example log:
This PR fixes the same