-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
HADOOP-17482: Remove Commons Logger from FileSystem Class #2633
Conversation
...-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/DelegationTokenRenewer.java
Show resolved
Hide resolved
...-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/DelegationTokenRenewer.java
Show resolved
Hide resolved
} | ||
// and at debug: the full stack | ||
LOG.debug("Stack Trace", ee); | ||
LOGGER.warn("Cannot load filesystem", ee); |
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'm not sure what the capability of Commons Logger is, but in SLF4J, the entire list of 'caused by' is printed so there is no need to do this manually when switching loggers, but this may also just be an oversight, I'm not sure.
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 think relying on slf4j and just dumping the whole exception is good.
|
||
{ | ||
try { | ||
GenericTestUtils.setLogLevel(FileSystem.LOG, Level.DEBUG); | ||
} | ||
catch(Exception e) { | ||
System.out.println("Cannot change log level\n" | ||
+ StringUtils.stringifyException(e)); | ||
} | ||
} |
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.
Not good practice to modify this manually. Should be using log4j properties files to set logging levels.
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.
its a test and they may have had some good reasons, just as some of the Azure tests do
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 need to fix it properly then. @belugabehr do you mind undoing this particular one in a separate JIRA or proposing the log4j solution?
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@goiri Thanks so much for the approval. Are you also able to do the merge? It's been a little while for me regarding the process and I'd need to research what the current practice/process is. |
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute