YARN-11836. Fixed AM log fetching with YARN CLI #7813
+29
−9
Conversation
This file contains hidden or 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
Change-Id: I04678a04503e01e67f1075f3fc543887cf80ac47
|
💔 -1 overall
This message was automatically generated. |
K0K0V0K
reviewed
Jul 21, 2025
| int haIndex = 0; | ||
| int activeRMIndex = 0; | ||
| int rmCount = 1; | ||
|
|
||
| if (HAUtil.isHAEnabled(conf)) { |
There was a problem hiding this comment.
What if we try some early return pattern?
if (!HAUtil.isHAEnabled(conf)) {
// If i understand right here we have just 1 RM so we can just try with that
String rmAddress = getRMWebAppURLWithScheme(conf, 0);
return func.apply(rmAddress, arg);
}
// try RMHAUtils.findActiveRMHAId(conf)
// if null do the iteration
// finally apply and return
Maybe will be a bit easyer to read cause now we have the
int activeRMIndex = 0;
int rmCount = 1;
code what related to non HA, than HA code, than some mix.
There was a problem hiding this comment.
Thanks, I agree. Fixed
| if (HAUtil.isHAEnabled(conf)) { | ||
| ArrayList<String> rmIds = (ArrayList<String>) HAUtil.getRMHAIds(conf); |
There was a problem hiding this comment.
Can we cast to simple List interface here? Maybe will be a bit more robust.
| String rmAddress = getRMWebAppURLWithScheme(conf, i); | ||
| return func.apply(rmAddress, arg); | ||
| } catch (Exception e) { | ||
| // Ignore and try next RM if there are any |
There was a problem hiding this comment.
Maybe some trace level LOG can be use full
Change-Id: Ia7a864fa77901dd63d193e0e9f0ccaab3674a75d
|
Thanks for your suggestions @K0K0V0K, I updated the patch with the fixes to your comments |
Change-Id: I35b0904be1c12e57349cc7a473a0d70ba38c6482
|
💔 -1 overall
This message was automatically generated. |
Change-Id: I76250565144f8f010ece1ef9d3781259fbae4c26
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
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.
Change-Id: I04678a04503e01e67f1075f3fc543887cf80ac47
Description of PR
YARN-10767 introduced a bug, where YARN Logs CLI is unable to fetch the AM logs using "-am" option if the user is not in the Admin ACLs.
This commit changed the logic for requesting the AM logs and it fetches the "id" of the active RM from the HA service, and requesting the logs from there.
Reproduction:
The issue can be reproduced by calling "yarn logs -applicationId ‹appId› -am 1" command with a user who has not got admin access.
In the RM logs of the test cluster I can see the following error, which states that the user doesn't have permission to call 'getServiceState':
Currently in WebAppUtils's execOnActiveRM method we throw an exception when RMHAUtils.findActiveRMHAId returns null here, stating that "No active RM is available". However that method will return null if the permissions are missing to check the service states. I think at this point we could fall back to the original code here, and try to find the active RM by iterating through them.
The issue only happens in HA mode, and only if we use "-am" option, without this option the AM logs can be retrieved together with the aggregated logs.
How was this patch tested?
Tested manually on a cluster by fetching application logs
The aggregated logs and AM logs could be fetched in every cases.
When I stopped the first RM, and the user could not check the active one, we found it when the connection timed out after 30 retries. With admin user it could find the active one for the first time
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?