[
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
