-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
YARN-11582. Improve WebUI diagnosticMessage to show AM Container resource request size. #6139
base: trunk
Are you sure you want to change the base?
Conversation
🎊 +1 overall
This message was automatically generated. |
@xiaojunxiang2023 Can we check if fairscheduler has this issue? We need to fix checkstyle. |
Good idea, tomorrow I will try to see if the fair scheduler has this problem. |
@Test | ||
public void testGetActivedAppDiagnosticMessage() throws IllegalAccessException, InstantiationException { | ||
StringBuilder diagnosticMessage = new StringBuilder( | ||
"Application is Activated, waiting for resources to be assigned for AM"); |
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.
indentation 5chars.
protected void getActivedAppDiagnosticMessage( | ||
StringBuilder diagnosticMessage) { | ||
diagnosticMessage.append(" Details : AM Partition = ") | ||
.append(" ; ") |
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.
5chars.
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 took a closer look at this unit test, which uses simple string concatenation for testing. I think this is not sufficient. Can we rewrite a more meaningful unit test?
Indeed, I will learn how to write test cases with context first, and I will catch up later |
🎊 +1 overall
This message was automatically generated. |
.../hadoop/yarn/server/resourcemanager/scheduler/capacity/TestApplicationLimitsByPartition.java
Outdated
Show resolved
Hide resolved
.../hadoop/yarn/server/resourcemanager/scheduler/capacity/TestApplicationLimitsByPartition.java
Outdated
Show resolved
Hide resolved
@xiaojunxiang2023 Thanks for the contribution! The code looks fine overall, with some minor modifications in the comments. |
0b78bfd
to
0eb1682
Compare
🎊 +1 overall
This message was automatically generated. |
0eb1682
to
d5f7e4f
Compare
🎊 +1 overall
This message was automatically generated. |
@slfan1989, Hi Sir,could you re-review my pr in your free time? if you approve it, please update the state, thanks. |
@xiaojunxiang2023 We need to fix the checkstyle issue. |
I have fixed the issue of indenting 5 characters during line breaks. Is there any other style issue? |
According to the hint, the only warning should be that the variable name is not proper. I just fixed the variable name and updated the PR, and am waiting for recompilation. |
💔 -1 overall
This message was automatically generated. |
8c24ef4
to
8066819
Compare
💔 -1 overall
This message was automatically generated. |
8066819
to
4089dfb
Compare
💔 -1 overall
This message was automatically generated. |
4089dfb
to
d61d4ce
Compare
🎊 +1 overall
This message was automatically generated. |
Hi,The latest PR has completely passed the checkstyle. Please review again if you are free. |
🎊 +1 overall
This message was automatically generated. |
@xiaojunxiang2023 Can we trigger jenkins compilation again? |
@hiwangzhihui Hi~, please help me review it. |
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.
Pr submission time is too long, you need to trigger Jenkins check again
d61d4ce
to
15b58c2
Compare
💔 -1 overall
This message was automatically generated. |
15b58c2
to
cf6d44d
Compare
💔 -1 overall
This message was automatically generated. |
cf6d44d
to
d0015be
Compare
🎊 +1 overall
This message was automatically generated. |
d0015be
to
9a2975e
Compare
💔 -1 overall
This message was automatically generated. |
…urce request size.
9a2975e
to
dca8ab0
Compare
🎊 +1 overall
This message was automatically generated. |
Assert.assertEquals(2, leafQueue.getNumActiveApplications()); | ||
String activatedDiagnostics="AM Resource Request = "; | ||
Assert.assertTrue("still doesn't show AMResource When Activated", app1.getDiagnostics() | ||
.toString().contains(activatedDiagnostics)); |
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.
Add a test checks for Am resource prompts would be better
jira:https://issues.apache.org/jira/browse/YARN-11582
When Yarn resources are insufficient, the newly submitted job AM may be in the state of "Application is Activated, waiting for resources to be assigned for AM". This is obviously because Yarn doesn't have enough resources to allocate another AM Container, so we want to know how large the AM Container is currently allocated. Unfortunately, the current diagnosticMessage on the Web page does not show this data. Therefore, it is necessary to add the resource size of the AM Container in the diagnosticMessage, which will be very useful for us to troubleshoise the production faults on line.
When I fix it later, look: the WebUI can successfully show the AM Container Request info when Application is in Activated State.