[ZEPPELIN-2815] Improve interpreter dependencies logging #2507
Conversation
|
Hi @Leemoonsoo and others, should be a trivial one. Low priority. Many thanks |
| @@ -122,7 +120,7 @@ public void transferSucceeded(TransferEvent event) { | |||
| @Override | |||
| public void transferFailed(TransferEvent event) { | |||
| transferCompleted(event); | |||
| event.getException().printStackTrace(out); | |||
| logger.warn(event.getException().getMessage()); | |||
There was a problem hiding this comment.
Warn and not error as it might be legit to not find a dependency in one repo (should fallback to the next)
There was a problem hiding this comment.
I'd convert to logger.warn(message, event.getException()) here
https://www.slf4j.org/api/org/slf4j/Logger.html#warn(java.lang.String,%20java.lang.Throwable)
| @@ -177,8 +177,9 @@ public synchronized void copyLocalDependency(String srcPath, File destPath) | |||
| DependencyFilterUtils.andFilter(exclusionFilter, classpathFilter)); | |||
| try { | |||
| return system.resolveDependencies(session, dependencyRequest).getArtifactResults(); | |||
| } catch (NullPointerException ex) { | |||
| throw new RepositoryException(String.format("Cannot fetch dependencies for %s", dependency)); | |||
| } catch (Exception ex) { | |||
There was a problem hiding this comment.
should be still NPE? since Exception can cache more exception types.
There was a problem hiding this comment.
Good question. I changed it to Exception as I see that org.sonatype.aether.RepositorySystem can throw DependencyResolutionException and I suspect we can have file exceptions and other in the whole org.sonatype.aether library. But because I didn't actually get an error here, I'll leave the final decision to you. Should we revert this to only have NPE or keep it broad with Exception?
There was a problem hiding this comment.
I think more specific exception would be preferred...
There was a problem hiding this comment.
Now
catch (NullPointerException | DependencyResolutionException ex)
| @@ -55,13 +53,13 @@ public void transferInitiated(TransferEvent event) { | |||
| @Override | |||
| public void transferProgressed(TransferEvent event) { | |||
| TransferResource resource = event.getResource(); | |||
| downloads.put(resource, Long.valueOf(event.getTransferredBytes())); | |||
There was a problem hiding this comment.
Don't we need converting? (LINE 62 as well)
There was a problem hiding this comment.
No. It's already a long
There was a problem hiding this comment.
Both are already a long I should say
| @@ -177,8 +177,9 @@ public synchronized void copyLocalDependency(String srcPath, File destPath) | |||
| DependencyFilterUtils.andFilter(exclusionFilter, classpathFilter)); | |||
| try { | |||
| return system.resolveDependencies(session, dependencyRequest).getArtifactResults(); | |||
| } catch (NullPointerException ex) { | |||
| throw new RepositoryException(String.format("Cannot fetch dependencies for %s", dependency)); | |||
| } catch (Exception ex) { | |||
There was a problem hiding this comment.
I think more specific exception would be preferred...
| @@ -135,10 +133,10 @@ private void transferCompleted(TransferEvent event) { | |||
|
|
|||
| @Override | |||
| public void transferCorrupted(TransferEvent event) { | |||
| event.getException().printStackTrace(out); | |||
| logger.error(event.getException().getMessage()); | |||
There was a problem hiding this comment.
logger.error("some message", event.getException()); instead
|
@1ambda , @felixcheung , ready for another round of review. Thanks |
| @@ -122,7 +120,7 @@ public void transferSucceeded(TransferEvent event) { | |||
| @Override | |||
| public void transferFailed(TransferEvent event) { | |||
| transferCompleted(event); | |||
| event.getException().printStackTrace(out); | |||
| logger.warn(event.getException().getMessage()); | |||
There was a problem hiding this comment.
I'd convert to logger.warn(message, event.getException()) here
https://www.slf4j.org/api/org/slf4j/Logger.html#warn(java.lang.String,%20java.lang.Throwable)
|
Makes sense @felixcheung . Changed warn message to |
|
LGTM and merge to master if no further discussions. |
What is this PR for?
As a developer, I want to be able to debug issues on dependency resolution "easier".
To achieve this, making available (in debug mode), existing listeners.
Also, exposing exceptions on dependency resolver.
What type of PR is it?
Improvement
What is the Jira issue?
How should this be tested?
Enable DEBUG logging mode. Import dependencies. Check detailed info. from dependency loading being logged.
Questions: