YARN-11726: Add logging statements for successful and unsuccessful password retrieval operation #7148
+26
−5
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
…ul password retrieval operations
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
K0K0V0K
suggested changes
Nov 11, 2024
There was a problem hiding this comment.
Thanks @Hean-Chhinling for the PR!
I hade some minor comments, but the PR looks mostly fine for me.
| } | ||
| } | ||
| catch (IOException ioe) { | ||
| password = null; | ||
| LOG.error("Unable to retrieve password for alias: {}", alias, ioe); |
There was a problem hiding this comment.
If i see well here the void error(String var1, Object var2, Object var3); called, so the 3th throwable wont be logged in this case. Also in this case two error log will be created, so maybe debug level will be also fine here.
There was a problem hiding this comment.
Hello @K0K0V0K,
Thank you for your feedbacks. By default SLF4J logging will handle the Throwable as the last parameter for exception logging. Therefore, I do not need another place {} holder for it.
There was a problem hiding this comment.
I tried with a DEBUG level log and log message does not appear when I run the unit test that simulate the Exception. I think it better with an ERROR level because now I changed when the password is null it log with WARN level. So when the exception happens, it will log with two log messages in two levels ERROR and WARN.
| @@ -509,11 +512,18 @@ static String getPassword(Configuration conf, String alias) { | |||
| char[] passchars = conf.getPassword(alias); | |||
| if (passchars != null) { | |||
| password = new String(passchars); | |||
| LOG.info("Successful password retrieval for alias: {}", alias); | |||
There was a problem hiding this comment.
I think debug level will be enough here, to avoid too much log record.
There was a problem hiding this comment.
I am not so sure, because when I try with DEBUG level and run the unit test the log message is not shown. I think it is override by other log level that is why I used INFO level to see it every time.
Is there a way to see a DEBUG level logs in some cases when the user wants?
...op-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/webapp/util/WebAppUtils.java
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of PR
This pull request enhances the
getPasswordmethod in the WebAppUtils class within Hadoop YARN by adding logging for password retrieval operations. Previously, if the password retrieval failed due to a misconfiguration or other issues, it failed silently, providing no indication of the error. This update adds:These changes improve transparency and traceability, making it easier for administrators to identify misconfigurations or other issues in YARN's password management.
How was this patch tested?
The changes were tested using the unit tests for the
getPasswordmethod, and I added newgetPasswordIOExceptionfor checking the error log when exception happens. These methods performed successfully showing the added logging functionality. No additional integration tests were required since this change only affects logging behavior.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?