[ https://issues.apache.org/jira/browse/YARN-9781?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16963916#comment-16963916 ]
Szilard Nemeth commented on YARN-9781: -------------------------------------- Hi [~prabhujoseph]! Thanks for this patch! Here are my comments: 1. I don't really like the boolean-flag based approach in {{org.apache.hadoop.yarn.client.cli.SchedConfCLI#run}}: Everything is controlled with boolean flags here. The flag {{hasOption}} is set to true in each of the if-clauses just to make the condition below the hasOption-conditions happy. This is bad. What I don't like is that this flag is set to true even for parameter that don't have an option (like your new one, 'getConf') at all, this is very misleading and hard to understand for the first read. As the code was in a bad shape before your patch as well, I can accept your patch, but please file a refactor jira that: a. Eliminates the {{hasOption}} boolean flag b. Where an option is misused, fail-fast: Have a method that contains this code and call it for every option, in-place: {code:java} if (!hasOption) { System.err.println("Invalid Command Usage: "); printUsage(); return -1; }{code} c. Remove the boolean flags: {{format}} and {{getConf}} as well. These are unnecessary. Note: For this patch, you don't need to do anything with bullet point 1. 2. There's a duplicated code block in {{SchedConfCLI#getSchedulerConf}}: {code:java} Configuration conf = getConf(); SSLFactory clientSslFactory = null; if (YarnConfiguration.useHttps(conf)) { clientSslFactory = new SSLFactory(SSLFactory.Mode.CLIENT, conf); } Client webServiceClient = createWebServiceClient(clientSslFactory); ClientResponse response = null; resource = (resource != null) ? resource : webServiceClient.resource(webAppAddress);{code} This seems to be copied from {{SchedConfCLI#formatSchedulerConf}}. Please extract this to a private helper method. 3. Nit: In {{SchedConfCLI#getSchedulerConf}}, {code:java} Builder builder = null;{code} could be: {code:java} Builder builder;{code} 4. Nit: {{org.apache.hadoop.yarn.client.cli.SchedConfCLI#prettyFormat}} could be private. 5. Nit: {{org.apache.hadoop.yarn.client.cli.SchedConfCLI#prettyFormat}} could be named prettyFormatWithIndent instead. 6. In: {{org.apache.hadoop.yarn.client.cli.TestSchedConfCLI#testGetSchedulerConf}}, the following code block is the exact copy from the method {{testFormatSchedulerConf}}: {code:java} if (rm != null) { rm.stop(); } CONF_FILE.delete(); if (OLD_CONF_FILE.exists()) { if (!OLD_CONF_FILE.renameTo(CONF_FILE)) { throw new RuntimeException("Failed to re-copy old" + " configuration file"); } } super.tearDown();{code} Please extract this to a private helper method! Moreover, after extraction, please check the result of the {code:java} CONF_FILE.delete();{code} call with an assertion. If deleting the file doesn't work, the test should fail immediately. 7. Nit: In: {{org.apache.hadoop.yarn.client.cli.TestSchedConfCLI#testGetSchedulerConf}}: Please add an assertion message to: {code:java} assertEquals(0, exitCode);{code} Thanks! > SchedConfCli to get current stored scheduler configuration > ---------------------------------------------------------- > > Key: YARN-9781 > URL: https://issues.apache.org/jira/browse/YARN-9781 > Project: Hadoop YARN > Issue Type: Sub-task > Components: capacity scheduler > Affects Versions: 3.3.0 > Reporter: Prabhu Joseph > Assignee: Prabhu Joseph > Priority: Major > Attachments: YARN-9781-001.patch, YARN-9781-002.patch, > YARN-9781-003.patch, YARN-9781-004.patch, YARN-9781-005.patch > > > SchedConfCLI currently allows to add / remove / remove queue. It does not > support get configuration which RMWebServices provides as part of YARN-8559. -- This message was sent by Atlassian Jira (v8.3.4#803005) --------------------------------------------------------------------- To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org