[NC-1675] Reduce log levels #133
Conversation
|
|
||
| public class DetermineCommonAncestorTask<C> extends AbstractEthTask<BlockHeader> { | ||
| private static final Logger LOG = getLogger(); |
There was a problem hiding this comment.
in some of these classes I'd put LogManager.getLogger(), in the future would it be better if I just out getLogger()?
There was a problem hiding this comment.
I prefer LogManager.getLogger. It gives a bit of context as to where the call comes from. After devcon I will likely do a large scale change and write an errorprone to pick a style and enforce it.
There is an errorprone check in the next release that checks for "confusing static imports." So ImmutableList.of cannot be statically imported. I don't know if getLogger was on the list but this one is common enough I think we should treat it as such.
There was a problem hiding this comment.
I also prefer LogManager.getLogger() and will go have a stern word with my IDE.
| LOG.warn( | ||
| "Received Wire DISCONNECT, but unable to parse reason. Peer: {}", | ||
| LOG.debug( | ||
| "Received Wire DISCONNECT with invalid RLP. Peer: {}", |
There was a problem hiding this comment.
I think its actually valid to send a disconnect message with no reason. We currently assume there is always a reason, so we get a parsing error when there is none. See: https://github.com/ethereum/devp2p/blob/master/devp2p.md:
reason is an optional integer specifying one of a number of reasons for disconnect
Might be worth adding a comment here and logging an issue.
There was a problem hiding this comment.
Good spot - I was wondering why so many clients were sending apparently invalid messages.
|
|
||
| public class DetermineCommonAncestorTask<C> extends AbstractEthTask<BlockHeader> { | ||
| private static final Logger LOG = getLogger(); |
There was a problem hiding this comment.
I prefer LogManager.getLogger. It gives a bit of context as to where the call comes from. After devcon I will likely do a large scale change and write an errorprone to pick a style and enforce it.
There is an errorprone check in the next release that checks for "confusing static imports." So ImmutableList.of cannot be statically imported. I don't know if getLogger was on the list but this one is common enough I think we should treat it as such.
PR description
To make logs more useful and less verbose, reduce log level to debug in a number of places where the behaviour is expected and doesn't need to be in the default log output.