[
https://issues.apache.org/jira/browse/YARN-4340?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15049724#comment-15049724
]
Subru Krishnan commented on YARN-4340:
--------------------------------------
Thanks [~seanpo03] for addressing my comments. This patch looks much better.
Please find my feedback below.
The Javadocs for *listReservations* API in *ApplicationClientProtocol* looks
pretty good. A few nits (same holds for Javadoc of *ReservationListRequest* and
*YarnClient*):
* We are still not explicitly calling out that the *ResourceAllocation* we
are returing in *ReservationListResponse* is based on the current state of the
Plan and we can change it for different reasons like replanning subject to the
constraints of the user contract as described by *ReservationDefinition*
* Specify the queue refers to the reservable queue in the scheduler. Refer
_ReservationSubmissionRequest_
* Remove username as it's no longer used
* Typo in starttime - start not end after the startTime will be selected
* Typo in endTime - end not start after the endTime will be selected
Minor Comments for rest of the patch:
* Replace all occurance of _plan_ with _queue_ in *ReservationListRequest*
* Typo in Javadoc of *ReservationAllocationState::getReservationDefinition*,
replace _set_ with _get_. Also you can link _ReservationDefinition_ in the
return param. This is cosmetic but I see it in a few places so if possible,
kindly fix those too.
* Can we order the arguments of *ResourceAllocationRequest::newInstance* as
_startTime, endTime and capability_ to improve readability
* In *ClientRMService::listReservations*, _includeResourceAllocations_ can be
of primitive boolean type
* Shouldn't we check for requestInfo.getEndTime() <= -1 in
*ClientRMService::listReservations*?
* Can we rename _info_ to say _reservationInfo_ or something more verbal to
make it more readable in *ClientRMService::listReservations*
* I am confused by the Javadocs of *PlanView::getReservations*, looks like
start and end time are swapped
* You can revert the change to *PlanView::getReservationByUserAtTime* as
that's being addressed in YARN-4358
* In *ReservationInputValidator*, the error strings can also be created in
*getPlanFromQueue* as they can be made consistent and reduce redundant code
* Can we rename _info_ to say _reservationInfo_ or something more verbal to
make it more readable in
*ReservationSystemUtil::convertAllocationsToReservationInfo*
* In *ReservationSystemUtil::convertAllocationsToReservationInfo*, we need
not create local variables for _acceptanceTime, user, id, definition_ as we
don't use them locally
* The _Map<ReservationInterval, Resource>_ can be defined outside of the for
loops in *ReservationSystemUtil::convertAllocationsToReservationInfo*
* In *TestClientRMService*, can you add a query for _listReservations_ based
on arrival and duration like you have done in *TestYarnClient*
* The test case additions to *TestInMemoryPlan* are great. Can we cover a few
more scenarios like more than reservations, invalid reservation id and boundary
conditions of time interval
* Looks like there are some whitespaces in *TestInMemoryPlan*
> Add "list" API to reservation system
> ------------------------------------
>
> Key: YARN-4340
> URL: https://issues.apache.org/jira/browse/YARN-4340
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: capacityscheduler, fairscheduler, resourcemanager
> Reporter: Carlo Curino
> Assignee: Sean Po
> Attachments: YARN-4340.v1.patch, YARN-4340.v2.patch,
> YARN-4340.v3.patch, YARN-4340.v4.patch, YARN-4340.v5.patch,
> YARN-4340.v6.patch, YARN-4340.v7.patch
>
>
> This JIRA tracks changes to the APIs of the reservation system, and enables
> querying the reservation system on which reservation exists by "time-range,
> reservation-id".
> YARN-4420 has a dependency on this.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
