Wangda Tan commented on YARN-3866:

Hi [~mding],
Thanks for working on the patch, some comments:

1) Test cases, I think most of the test cases changes are not necessary, we 
have automatical PB tests verification, I think adding them to 
TestPBImplRecords should be enough. Addition tests are needed only if there's 
any special logic that auto PB test cannot do.
2) Java docs annoatation:
Some general suggestions to do annoation
- Only need to mark user interface (for example, RPC between service daemons 
doesn't need to be marked)
- For request. For example, {{AllocateRequest}}, {{ResourceRequest}}, which 
user needs to set values, put @Public for both setter/getter/newInstance
- For response. For example, {{AllocateResponse}}, {{IncreasedContainer}}, 
which user doesn't need to set values, put @Public to getter only, and @Private 
to setter/newInstance.
- @Stable/@Unstable need to be added for both @Public/@Private. @Private always 
follows by @Unstable. If you think a @Public interface could be changed, put 
@Unstable to it. Otherwise, put @Stable.

*Code style:*
1) Could you check some of indents in your code? I think 8 spaces indent is too 
    Iterable<ContainerResourceChangeRequestProto> iterable =
            new Iterable<ContainerResourceChangeRequestProto>() {

Hadoop typically uses 2-4 indents for wrapped code.

Other parts of the patch looks

> AM-RM protocol changes to support container resizing
> ----------------------------------------------------
>                 Key: YARN-3866
>                 URL: https://issues.apache.org/jira/browse/YARN-3866
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: api
>            Reporter: MENG DING
>            Assignee: MENG DING
>         Attachments: YARN-3866.1.patch
> YARN-1447 and YARN-1448 are outdated. 
> This ticket deals with AM-RM Protocol changes to support container resize 
> according to the latest design in YARN-1197.
> 1) Add increase/decrease requests in AllocateRequest
> 2) Get approved increase/decrease requests from RM in AllocateResponse
> 3) Add relevant test cases

This message was sent by Atlassian JIRA

Reply via email to