-
Notifications
You must be signed in to change notification settings - Fork 9.2k
YARN-11159. Support failApplicationAttempt, updateApplicationPriority, updateApplicationTimeouts API's for Federation #4396
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
Conversation
…, updateApplicationTimeouts API's for Federation.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| LOG.error("Unable to update application priority for {} " + | ||
| "to SubCluster {}.", | ||
| request.getApplicationId(), subClusterId.getId(), e); | ||
| throw 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.
logAndThrowException()?
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.
Thanks for your help reviewing the code, your suggestion is correct, I will fix it.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Please help me review the code again, thank you very much, I will follow up on YARN-10122. Can you help merge to trunk branch? |
|
@goiri Please help me review the code again, thank you very much! |
|
|
||
| private SubmitApplicationRequest mockSubmitApplicationRequest( | ||
| ApplicationId appId) { | ||
| ApplicationId appId, int priority) { |
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.
Can we add one with APP_PRIORITY_ZERO as default instead of having to modify every call?
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.
No problem, I will fix the code.
| () -> interceptor.failApplicationAttempt(null)); | ||
|
|
||
| // normal request | ||
| ApplicationId appId = |
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.
Single line should fit within limit.
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 will fix the code.
| interceptor.getApplicationAttempts(attemptsRequest); | ||
|
|
||
| // Wait for app to start | ||
| while(attemptsResponse.getApplicationAttemptList().size() == 0) { |
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.
To avoid an active wait, we should do a waitFor() or similar.
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.
Thanks for the suggestion, I will modify this part of the code.
| Assert.assertNotNull(attemptsResponse); | ||
|
|
||
| String formatISO8601 = | ||
| Times.formatISO8601(System.currentTimeMillis() + 5 * 1000); |
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.
Spacing
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 will fix the code.
|
@goiri Please help me review the code again, thank you very much! |
|
🎊 +1 overall
This message was automatically generated. |
| interceptor.getApplicationAttempts(attemptsRequest); | ||
| Assert.assertNotNull(attemptsResponse); | ||
|
|
||
| String formatISO8601 = |
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.
This probably should have "time" in the var name.
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.
Thanks for the help reviewing the code, I will fix it.
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Thanks for your help to review the code! |
…, updateApplicationTimeouts API's for Federation (apache#4396)
JIRA: YARN-11159. Support failApplicationAttempt, updateApplicationPriority, updateApplicationTimeouts API's for Federation