I am experiencing an issue as it is where the response JSON gets improperly marshalled for single element arrays. For example, if the response data is an array of objects JSON serialization will correctly produce the following:

[

].

If the array only contains a single response, the following will be the output of the serialization:

.

I'm not sure why this problem occurs, but this effect cannot be seen when using the CapacityScheduler (ws/v1/cluster/scheduler) API. Any information will be very helpful.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

For the following, the fields are required JSON serialization:
Unread field:ReservationDefinitionInfo.java:[line 36]
Unread field:ReservationDefinitionInfo.java:[line 37]
Unread field:ReservationDefinitionInfo.java:[line 38]
Unread field:ReservationDefinitionInfo.java:[line 45]
Unread field:ReservationInfo.java:[line 48]
Unread field:ReservationInfo.java:[line 49]
Unread public/protected field:At ReservationInfo.java:[line 45]
Unread public/protected field:At ReservationRequestInfo.java:[line 35]
Unread public/protected field:At ReservationRequestInfo.java:[line 36]
Unread public/protected field:At ReservationRequestInfo.java:[line 37]
Unread public/protected field:At ReservationRequestInfo.java:[line 38]
Unread public/protected field:At ReservationRequestsInfo.java:[line 37]
Unread public/protected field:At ReservationRequestsInfo.java:[line 38]

The following test might be flaky - I am not able to reproduce this test failure:
hadoop.yarn.server.nodemanager.containermanager.localizer.TestResourceLocalizationService

I do not think the following are tests that are caused by my changes as they also occur in the latest version of trunk.
hadoop.yarn.server.resourcemanager.TestAMAuthorization
hadoop.yarn.server.resourcemanager.TestClientRMTokens
hadoop.mapreduce.v2.TestMRJobsWithProfiler

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

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.

Thinking more about it, I am not sure we should have user in the interface. We should automatically pick up user from context. If we want to allow user, then it should be an Admin API and not a Client API IMHO.

I took all your suggestions above, and with regards to your second comment, I also removed 'user' from the interface in v5 of the patch.
The REST aspect of the code was also removed, and will be a part of a second ticket YARN-4420.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Fixed all relevant checkstyle, javac, findbugs, javadoc issues.

-1 overall

This message was automatically generated.

This patch addresses the remaining findbugs, checkstyle errors.

The following unit tests are failing and have associated Jira tickets:
hadoop.yarn.server.resourcemanager.TestClientRMTokens
– YARN-4306
hadoop.yarn.server.resourcemanager.TestAMAuthorization
– YARN-4318
hadoop.yarn.client.TestGetGroups
– YARN-4351

The following unit tests are passing locally and flakiness may be related to YARN-4352:
org.apache.hadoop.yarn.client.api.impl.TestYarnClient
– testShouldNotRetryForeverForNonNetworkExceptions also fails locally on trunk
– testAMMRToken passes locally on trunk
org.apache.hadoop.yarn.client.api.impl.TestAMRMClient
org.apache.hadoop.yarn.client.api.impl.TestNMClient

The following tests are passing locally:
hadoop.mapreduce.v2.TestMRJobsWithProfiler

-1 overall

This message was automatically generated.

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

Thanks Subru for the comments. I have taken all your recommendations, except for the following:

The startTime and endTime wasn't a typo - I actually intended on selecting all active reservations within the time interval. that's why only reservations that ended after the selection startTime, and reservations that start before or on the selection endTime are included in the response.

In ReservationInputValidator, I didn't want to create the error strings from getPlanFromQueue because it almost seems like we are bending over backwards to prevent repeated code. The reason is because validateReservation uses this method, and has a drastically different error message, and it doesn't seem easy to reword it to fit into the pattern of the other error messages. I have instead overloaded the method so that users can optionally pass in the error messages. Otherwise, the defaults will be used.

-1 overall

This message was automatically generated.

Thanks for addressing my feedback Sean Po. The latest patch (v8) LGTM.

Carlo Curino, can you take a look and commit.

Sean Po, I noticed there are couple of minor checkstyle & Javadoc issues. Can you kindly fix those.

Also the TestReservationInputValidator::testSubmitReservationDoesnotExist is failing, seems like a minor fix.

It will be great if you can try running it e2e on at least a single node setup.

Thanks for working on this.

Sean Po, I ran a few e2e tests on my single box and found the following small issues:

• Can we have newInstance methods ReservationListRequest with just the queue and reservationid as that will make it easy for the most common cases
• We need a null check for reservationid in ReservationListRequest
• I see the reservation being listed even though I give the start time after the end of the reservation
• Similarly I see the reservation being listed even though I give the end time before the start of the reservation
• We need to add a check for endTime > startTime in ReservationInputValidator

Can you also unit tests to cover these scenarios.

Thanks!

Thanks Subru for the comments. I took your comments, and added it to this latest patch. After doing that, I noticed that I am not able to replicate the incorrect behavior that you mentioned.

-1 overall

This message was automatically generated.

Fixing Checkstyle Issues.

-1 overall

This message was automatically generated.

Added null check for reservationId in ClientRMService.

-1 overall

This message was automatically generated.

I made a comment about the test failures before, but to make it easier to find, I'll paste it here again:

This patch addresses the remaining findbugs, checkstyle errors.
The following unit tests are failing and have associated Jira tickets:
hadoop.yarn.server.resourcemanager.TestClientRMTokens
– YARN-4306
hadoop.yarn.server.resourcemanager.TestAMAuthorization
– YARN-4318
hadoop.yarn.client.TestGetGroups
– YARN-4351
The following unit tests are passing locally and flakiness may be related to YARN-4352:
org.apache.hadoop.yarn.client.api.impl.TestYarnClient
– testShouldNotRetryForeverForNonNetworkExceptions actually fails, but it also fails locally on trunk
– testAMMRToken
org.apache.hadoop.yarn.client.api.impl.TestAMRMClient
org.apache.hadoop.yarn.client.api.impl.TestNMClient
The following tests are passing locally:
hadoop.mapreduce.v2.TestMRJobsWithProfiler

There are three new test failures above, but when I ran the tests locally, they all passed:
hadoop.mapred.TestJobSysDirWithDFS
hadoop.mapred.TestNetworkedJob
hadoop.yarn.server.resourcemanager.scheduler.fifo.TestFifoScheduler

If you also look at the Hadoop QA comment from two days ago, where the code only changed with regards to style, there were very few test failures, which indicates that some of the above tests are flaky:

JDK v1.8.0_66 Failed junit tests
hadoop.yarn.server.resourcemanager.TestAMAuthorization
hadoop.yarn.server.resourcemanager.scheduler.fifo.TestFifoScheduler
hadoop.yarn.server.resourcemanager.TestClientRMTokens

Thanks Sean Po,
Patch looks generally good to me, few minor comments: