This repository has been archived by the owner on Sep 26, 2019. It is now read-only.
Peer disconnects should not result in stack traces. #818
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replace two known an info level stack trace would occur with a single debug line because disconnects are basically expected.
mbaxter
reviewed
Feb 8, 2019
| @@ -105,6 +106,10 @@ public ChainDownloader( | |||
| LOG.trace("Download cancelled", t); | |||
| } else if (rootCause instanceof InvalidBlockException) { | |||
| LOG.debug("Invalid block downloaded", t); | |||
| } else if (rootCause instanceof PeerDisconnectedException) { | |||
There was a problem hiding this comment.
Suggested change
| } else if (rootCause instanceof PeerDisconnectedException) { | |
| } else if (rootCause instanceof EthTaskException) { | |
| LOG.debug(rootCause.getMessage()); | |
| } |
There was a problem hiding this comment.
While you're at it, you could probably just handle all EthTaskException's ^ as those are "expected" exceptions. It seems that LOG.error below should be there to catch unexpected exceptions.
There was a problem hiding this comment.
Yep. We don't need stack traces when we fail in expected ways.
These exceptions fall into the category of flow control instead of errors, hence the lack of stacktrace.
shemnon
commented
Feb 8, 2019
| @@ -105,6 +106,8 @@ public ChainDownloader( | |||
| LOG.trace("Download cancelled", t); | |||
| } else if (rootCause instanceof InvalidBlockException) { | |||
| LOG.debug("Invalid block downloaded", t); | |||
| } else if (rootCause instanceof EthTaskException) { | |||
| LOG.debug(rootCause.toString()); | |||
There was a problem hiding this comment.
toString is deliberate, to show we are not attempting to log the throwable.
mbaxter
approved these changes
Feb 8, 2019
| @@ -550,7 +550,7 @@ public void recoversFromSyncTargetDisconnect() { | |||
| // Downloader should recover and sync to next best peer, but it may stall | |||
| // for 10 seconds first (by design). | |||
| await() | |||
| .atMost(20, TimeUnit.SECONDS) | |||
| .atMost(30, TimeUnit.SECONDS) | |||
There was a problem hiding this comment.
Did you mean to include this change here?
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR description
Replace two known stack traces an info level. Replace with a single
debug line because disconnects are basically expected.