Zhijie Shen commented on YARN-2629:

bq. 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?

Previously, the exception was handled at the caller of publishXXXX(), but I 
moved into publishXXXX() to simplify the change, such that there's no need to 
throw the exception any more. Usually, it's a good behavior to handle each 
potential exception explicitly. Here since I intentionally don't want to do 
such fine-grained handling, I choose to log whatever the exception is.

bq. 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)?

Added one

bq. This variable name looks unclear. Maybe we want to rename it so that people 
can easily connect it with timeline domains?

It's explained in the usage of the option and have be commented:
  // Flag to indicate whether to create the domain of the given ID
  private boolean toCreate = false;

but let me elaborate this var in code.

> 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