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