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

Wangda Tan edited comment on YARN-5952 at 3/27/17 10:29 PM:
------------------------------------------------------------

Thanks [~jhung], 

Had some offline discussions with [~vinodkv], some comments: 

1)
For REST end point, do you think is it better to rename it to {{/queues}}? I 
think what this feature done is mostly for queue-related config changes. In the 
future we could add changes to modify scheduler global configs as well, but we 
can do that though a different end point since it doesn't have the issues same 
to queue-related configs like atomically update/add/remove queues. 

And an additional benefit is we can have a GET {{/queues}} and 
{{/queues/{queue-path}}} in the future which looks more consistent.

2)
For update queue REST record:
2.1) Is it better to rename {{SchedulerConfigurationInfo}} to 
{{QueueConfigsUpdate}}?
2.2) I'm not sure if the nested queue hierarchy is a MUST for your use cases, 
from our POV it is a little confusing. Could we flat it and for all queues 
requires to specify full queue path?
2.3) For general REST layout. Here's my suggested REST record: 

{code}
QueueConfigsUpdate: {
        adds: [
                {
                        queuePath: "root.a",
                        params: {
                                "x": "x1",
                                "y": "y1"
                        }               
                },
                {
                        queuePath: "root.a.a1",
                        params: {
                                // ...
                        }
                }
        ],
        removes: [
                "root.b",
                "root.c"
        ],
        updates: [
                // internal of updates is same as adds
        ]
}
{code}

Basically we can reuse REST Java record of add/update. And removes could be a 
simple array of Strings.

3) ACLs:

We can add more per-queue ACLs in the future, but I think for this patch, we 
should only allow admin to update configs. You can reference to logics in 
{{dumpSchedulerLogs}}:

{code}
    ApplicationACLsManager aclsManager = rm.getApplicationACLsManager();
    if (aclsManager.areACLsEnabled()) {
      if (callerUGI == null || !aclsManager.isAdmin(callerUGI)) {
        String msg = "Only admins can carry out this operation.";
        throw new ForbiddenException(msg);
      }
    }
{code}

4) Instead of checking if a scheduler is CS or not, is it better to make CS 
inheriting an interface like {{MutableConfScheduler}} and use instanceof to 
check scheduler in RMWebServices.


was (Author: leftnoteasy):
Thanks Jonathan, 

Had some offline discussions with @vinod, some comments: 

1)
For REST end point, do you think is it better to rename it to {{/queues}}? I 
think what this feature done is mostly for queue-related config changes. In the 
future we could add changes to modify scheduler global configs as well, but we 
can do that though a different end point since it doesn't have the issues same 
to queue-related configs like atomically update/add/remove queues. 

And an additional benefit is we can have a GET {{/queues}} and 
{{/queues/{queue-path}}} in the future which looks more consistent.

2)
For update queue REST record:
2.1) Is it better to rename {{SchedulerConfigurationInfo}} to 
{{QueueConfigsUpdate}}?
2.2) I'm not sure if the nested queue hierarchy is a MUST for your use cases, 
from our POV it is a little confusing. Could we flat it and for all queues 
requires to specify full queue path?
2.3) For general REST layout. Here's my suggested REST record: 

{code}
QueueConfigsUpdate: {
        adds: [
                {
                        queuePath: "root.a",
                        params: {
                                "x": "x1",
                                "y": "y1"
                        }               
                },
                {
                        queuePath: "root.a.a1",
                        params: {
                                // ...
                        }
                }
        ],
        removes: [
                "root.b",
                "root.c"
        ],
        updates: [
                // internal of updates is same as adds
        ]
}
{code}

Basically we can reuse REST Java record of add/update. And removes could be a 
simple array of Strings.

3) ACLs:

We can add more per-queue ACLs in the future, but I think for this patch, we 
should only allow admin to update configs. You can reference to logics in 
{{dumpSchedulerLogs}}:

{code}
    ApplicationACLsManager aclsManager = rm.getApplicationACLsManager();
    if (aclsManager.areACLsEnabled()) {
      if (callerUGI == null || !aclsManager.isAdmin(callerUGI)) {
        String msg = "Only admins can carry out this operation.";
        throw new ForbiddenException(msg);
      }
    }
{code}

4) Instead of checking if a scheduler is CS or not, is it better to make CS 
inheriting an interface like {{MutableConfScheduler}} and use instanceof to 
check scheduler in RMWebServices.

> Create REST API for changing YARN scheduler configurations
> ----------------------------------------------------------
>
>                 Key: YARN-5952
>                 URL: https://issues.apache.org/jira/browse/YARN-5952
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Jonathan Hung
>            Assignee: Jonathan Hung
>         Attachments: YARN-5952.001.patch, YARN-5952.002.patch, 
> YARN-5952-YARN-5734.003.patch, YARN-5952-YARN-5734.004.patch, 
> YARN-5952-YARN-5734.005.patch
>
>
> Based on the design in YARN-5734.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

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

Reply via email to