Thanks Sean Po 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.