hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Subru Krishnan (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-4340) Add "list" API to reservation system
Date Thu, 10 Dec 2015 00:31:11 GMT

    [ 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)

Mime
View raw message