[ 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