[jira] [Comment Edited] (YARN-5552) Add Builder methods for common yarn API records

[Wangda Tan (JIRA)]

[ 
https://issues.apache.org/jira/browse/YARN-5552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15583572#comment-15583572
 ] 

Wangda Tan edited comment on YARN-5552 at 10/17/16 9:40 PM:


Thanks [~Tao Jie].

Few minor comments from my side: 

1) New added methods of AllocateResponse should be all @Private and @Unstable, 
we should not mark original write-methods of AllocateResponse to @Public at the 
beginning, the reason is AllocateResponse should be read-only by YARN app, no 
YARN app should set these fields. YARN services should take care of settings 
these fields of AllocateResponse.

2) Can we merge common sanity check logic of ContainerRequestBuilder#build and 
ContainerRequest#constructor?

3) For all new added write methods of builder, it's better to add a javadocs 
link-to reference original method.


was (Author: leftnoteasy):
Thanks [~Tao Jie].

Few minor comments from my side: 

1) New added methods of AllocateResponse should be all @Private and @Unstable, 
we should not mark original write-methods of AllocateResponse to @Public at the 
beginning, the reason is AllocateResponse should be read-only, no YARN app 
should set these fields.

2) Can we merge common sanity check logic of ContainerRequestBuilder#build and 
ContainerRequest#constructor?

3) For all new added write methods of builder, it's better to add a javadocs 
link-to reference original method.

> Add Builder methods for common yarn API records
> ---
>
> Key: YARN-5552
> URL: https://issues.apache.org/jira/browse/YARN-5552
> Project: Hadoop YARN
>  Issue Type: Improvement
>Reporter: Arun Suresh
>Assignee: Tao Jie
> Attachments: YARN-5552.000.patch, YARN-5552.001.patch, 
> YARN-5552.002.patch, YARN-5552.003.patch, YARN-5552.004.patch, 
> YARN-5552.005.patch
>
>
> Currently yarn API records such as ResourceRequest, AllocateRequest/Respone 
> as well as AMRMClient.ContainerRequest have multiple constructors / 
> newInstance methods. This makes it very difficult to add new fields to these 
> records.
> It would probably be better if we had Builder classes for many of these 
> records, which would make evolution of these records a bit easier.
> (suggested by [~kasha])



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org

[jira] [Comment Edited] (YARN-5552) Add Builder methods for common yarn API records

[Tao Jie (JIRA)]

[ 
https://issues.apache.org/jira/browse/YARN-5552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15469779#comment-15469779
 ] 

Tao Jie edited comment on YARN-5552 at 9/7/16 6:57 AM:
---

Thank you for your comment,[~leftnoteasy]!
Storing local variables in builder here is to give a default value for fields 
and avoid potential NPE. It is better to set default value also in builder 
constructor like:
{code}
private ResourceRequestBuilder() {
  ResourceRequest request = Records.newRecord(ResourceRequest.class);
  request.setResourceName(ResourceRequest.ANY);
  request.setPriority(Priority.newInstance(0));
  ...
}
{code}


was (Author: tao jie):
Thank you for your comment,[~leftnoteasy]!
Storing local variables in builder here is to give a default value for fields 
and avoid potential NPE. It is better to set default value also in builder 
constructor like:
{quote}
private ResourceRequestBuilder() {
  ResourceRequest request = Records.newRecord(ResourceRequest.class);
  request.setResourceName(ResourceRequest.ANY);
  request.setPriority(Priority.newInstance(0));
  ...
}
{quote}

> Add Builder methods for common yarn API records
> ---
>
> Key: YARN-5552
> URL: https://issues.apache.org/jira/browse/YARN-5552
> Project: Hadoop YARN
>  Issue Type: Improvement
>Reporter: Arun Suresh
>Assignee: Tao Jie
> Attachments: YARN-5552.000.patch, YARN-5552.001.patch, 
> YARN-5552.002.patch, YARN-5552.003.patch
>
>
> Currently yarn API records such as ResourceRequest, AllocateRequest/Respone 
> as well as AMRMClient.ContainerRequest have multiple constructors / 
> newInstance methods. This makes it very difficult to add new fields to these 
> records.
> It would probably be better if we had Builder classes for many of these 
> records, which would make evolution of these records a bit easier.
> (suggested by [~kasha])



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org