-
Notifications
You must be signed in to change notification settings - Fork 897
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
Backward sync log UX improvements #4655
Conversation
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
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
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
5003ad7
to
0ad7d31
Compare
Signed-off-by: garyschulte <garyschulte@gmail.com>
@@ -439,7 +443,7 @@ public ForkchoiceResult updateForkChoice( | |||
|
|||
if (newHead.getNumber() < blockchain.getChainHeadBlockNumber() | |||
&& isDescendantOf(newHead, blockchain.getChainHeadHeader())) { | |||
LOG.info("Ignoring update to old head"); | |||
debugLambda(LOG, "Ignoring update to old head {}", newHead::toLogString); |
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.
"Ignoring head update {}", newHead
?
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.
we can be specific here, since we know that FcU is trying to set the head to an old block in the same chain of the current head and so we ignore it
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.
I was just wondering if it could be confusing that it says "old head" but what is inserted is the newHead
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.
what about renaming newHead
to something like tentativeNewHead
or possibleNewHead
to state that it is not yet confirmed?
...h/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java
Outdated
Show resolved
Hide resolved
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.
couple of non-blocking suggestions for wording
6dfada4
to
d8c4628
Compare
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
d801bde
to
6c0c302
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.
LGTM
...h/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: wcgcyx <wcgcyx@gmail.com>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio fabio.difabio@consensys.net
PR description
Improve the backward sync logging, showing only the progress and critical errors, and pushing transient errors to debug.
Progress logs are printed at least 10 sec apart, to avoid spamming the console
example of the new progress log
Fixed Issue(s)
relates to #4446
Documentation
doc-change-required
label to this PR ifupdates are required.
Changelog