[ 
https://issues.apache.org/jira/browse/YARN-4340?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15034792#comment-15034792
 ] 

Subru Krishnan commented on YARN-4340:
--------------------------------------

Thanks [~seanpo03] for working on this. Kindly fix the test-patch warnings.

I went through the patch and please find my feedback below. 

First up, the patch is huge and will only get bigger so I suggest splitting it 
into 2 - the RPC API changes and REST  API changes. I have reviewed only the 
RPC changes here.

Secondly, there are very few test cases. You need to test cases at least for 
_TestInMemoryPlan, TestYarnClient, TestReservationInputValidator, 
TestPBImplRecords_, etc. Take a look at the other Reservation APIs for 
reference.

Lastly, *YarnClient* needs to updated to add the new API so that it's 
consumable by clients.

Now to the new *listReservations* API in *ApplicationClientProtocol*. We should 
be very diligent as we are adding an externally visible API:
   * We should use _queue_ instead of _planName_ as _Plan_ is an internal data 
structure and we only expose queues to users
   * We should clearly explain in the Javadocs how the different params in 
*ReservationListRequest* influence the query. For e.g.: what are mandatory, 
what are optional, how does specifying multiple params affect the result, etc
   * We must explicitly call out 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*

Minor comments for rest of the code:
   * Typos in argument names for _setPlanName_ and _setUser_ in 
*ReservationListRequest*
   * Link the _Plan/ReservationId and ResourceAllocation_ in the Javadocs of 
*ReservationListRequest*
   * The Javadoc of *ReservationListResponse* has copy paste typo, please 
update it
   * In *ReservationInfo*, all setters should be _@Private_ 
   * Link the _ReservationDefinition/ReservationId and ResourceAllocation_ in 
the Javadocs of *ReservationInfo*
   * We should reconcile *ResourceAllocation* with 
*ReservationAllocationStateProto* which we use for recovery as both look to 
contain the same information
   * Rename *ResourceAllocation* --> *ReservationResourceAllocation*
   * Again call out explicitly in the Javadocs that the allocations are 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*
   * In *ResourceAllocation*, all setters should be _@Private_ 
   * Link _Resource_ in the Javadocs of *ResourceAllocation*
   * In *ReservationListRequestPBImpl*, return NULL if string field is not set, 
not empty string
   * In *ReservationListRequestPBImpl*, check for negative long values before 
setting. Use _ReservationDefinitionPBImpl_ as reference
   * Align *ReservationListRequestPBImpl::getReservationInfo* with 
_ReservationRequestsPBImpl::getReservationResources_
   * Add existence checks for getters and null/negative check for setters for 
fields in *ReservationInfoPBImpl*
   * Align *ReservationInfoPBImpl::getResourceAllocations* with 
_ReservationRequestsPBImpl::getReservationResources_
   * In *ResourceAllocationPBImpl*, check for negative long values before 
setting
   * Revert the unrelated formatting changes in *ClientRMService*
   * In *ClientRMService::listReservations*, ACL check has to be done before 
querying the _Plan_
   * In *ClientRMService::listReservations*, refactor the translation of 
_Set<ReservationAllocation>_ into _List<ReservationInfo>_ into a separate 
helper method. It may make sense to have in *ReservationSystemUtil* as can have 
other consumers in future
   * In *InMemoryPlan::getReservations*, there are 2 read locks - the outer one 
is redundant and can be removed
   * Add null check for _user_ in *InMemoryPlan::getReservations*
   * In *ReservationInputValidator::validateReservationListRequest*, the 
validation seems to be common with other existing checks so would be better to 
extract a common helper private method and reuse it.
   * In *ReservationInputValidator::validateReservationListRequest*, we are 
only validating the _plan queue_ name - what about other params?
   * In *TestClientRMService*, add more _listReservations_ queries after 
submission, deletion. Also check for a narrower time window rather than 0 - 
Long.MAX_VALUE.
   * In *TestClientRMService*, rename _lRequest_ and _lResponse_ with _request_ 
and _response_ respectively.

> 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
>
>
> 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, username".



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to