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 Tue, 01 Dec 2015 22:52:11 GMT

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

Mime
View raw message