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-11226. [Federation] Add createNewReservation, submitReservation, updateReservation, deleteReservation REST APIs for Router. #4892
Conversation
…Reservation, updateReservation, deleteReservation REST APIs for Router.
💔 -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. |
🎊 +1 overall
This message was automatically generated. |
interceptor.getOrCreateInterceptorForSubCluster( | ||
subCluster, subClusterInfo.getRMWebServiceAddress()); | ||
} | ||
} catch (YarnException 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.
Why isn't enough to surface the exception?
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, I will fix it.
|
||
try { | ||
interceptor.setupResourceManager(); | ||
} catch (Exception 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.
Surfacing the exception would be enough
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 it.
// Define default queue | ||
conf.setCapacity(QUEUE_DEFAULT_FULL, 20); | ||
// Define dedicated queues | ||
conf.setQueues(CapacitySchedulerConfiguration.ROOT, |
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
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 it.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri Please help to review the code again, thank you very much! |
🎊 +1 overall
This message was automatically generated. |
* @param reservationId reservationId | ||
* @return true - exist, false - not exist | ||
*/ | ||
public static Boolean existsReservationHomeSubCluster(FederationStateStoreFacade federationFacade, |
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 this be the native boolean
type?
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, I will modify the code.
ClientRMService clientService = mockRM.getClientRMService(); | ||
clientService.updateReservation(request); | ||
|
||
} catch (Exception ex) { |
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 is a fairly broad catch, wouldn't it be better to just throw the exceptions?
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 agree with you, I will modify the code.
|
||
ReservationRequestsInfo resReqsInfo = resInfo.getReservationRequests(); | ||
if (resReqsInfo == null || resReqsInfo.getReservationRequest() == null | ||
|| resReqsInfo.getReservationRequest().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.
isEmpty()
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 it.
ReservationId reservationId, int numContainers, long arrival, | ||
long deadline, long duration, int memory, int vcore) { | ||
// create a request with a single atomic ask | ||
ReservationRequest r = ReservationRequest |
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.
Small nit, this would look better as:
ReservationRequest r = ReservationRequest.newInstance(
Resource.newInstance(memory, vcore), numContainers, 1, duration);
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 fix it.
Collections.singletonList(r), ReservationRequestInterpreter.R_ALL); | ||
ReservationDefinition rDef = ReservationDefinition.newInstance(arrival, | ||
deadline, reqs, "testClientRMService#reservation", "0", Priority.UNDEFINED); | ||
ReservationSubmissionRequest request = ReservationSubmissionRequest |
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.
Same here for the break line
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 it.
@@ -51,4 +70,48 @@ protected void registerBadSubCluster(SubClusterId badSC) { | |||
interceptor.setRunning(false); | |||
} | |||
|
|||
protected void setupResourceManager() throws IOException { | |||
try { | |||
if (mockRM != null) { |
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.
You can move some of this before the try
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 it.
...test/java/org/apache/hadoop/yarn/server/router/webapp/TestableFederationInterceptorREST.java
Show resolved
Hide resolved
ReservationRequests reservationRequests = | ||
ReservationRequests.newInstance(reservationRequestList, reservationRequestInterpreter); | ||
|
||
ReservationDefinition definition = |
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.
First two lines into one line.
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 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.
This?
...-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/RouterServerUtil.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri Please help to review the code again, thank you very much! |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri Can you help review this pr again? thank you very much! |
💔 -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 Can you help merge this pr into trunk branch? Thank you very much! I will follow up with YARN-11320. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
JIRA: YARN-11226. [Federation] Add createNewReservation, submitReservation, updateReservation, deleteReservation REST APIs for Router.