Li Lu commented on YARN-2629:

Hi [~zjshen], I reviewed this patch (YARN-2629.3.patch), and it generally LGTM. 
However, I do have a few quick comments:

1. I noticed that you've changed a few method signatures, and removed the 
"throws IOException, YarnException" clause from the signatures. Meanwhile, in 
the method I noticed that you're catching all types of exceptions in one catch 
statement. I agree that this change saves some repeated lines of try-catch 
statements in various call sites, but is it OK catch all exceptions internally? 
 (I think so, but would like to verify that. )

2. Maybe we want to add one more test with no arguments on 
domain/view_acl/modify_acl? Timeline domain is a newly added feature, and maybe 
we'd like to make sure it would not break any existing distributed shell usages 
(esp. in some scripts)? 

3. In Client.java:
private boolean toCreate = false;
This variable name looks unclear. Maybe we want to rename it so that people can 
easily connect it with timeline domains?

> Make distributed shell use the domain-based timeline ACLs
> ---------------------------------------------------------
>                 Key: YARN-2629
>                 URL: https://issues.apache.org/jira/browse/YARN-2629
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Zhijie Shen
>            Assignee: Zhijie Shen
>         Attachments: YARN-2629.1.patch, YARN-2629.2.patch, YARN-2629.3.patch
> For demonstration the usage of this feature (YARN-2102), it's good to make 
> the distributed shell create the domain, and post its timeline entities into 
> this private space.

This message was sent by Atlassian JIRA

Reply via email to