[
https://issues.apache.org/jira/browse/YARN-9699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16947461#comment-16947461
]
Peter Bacsko commented on YARN-9699:
------------------------------------
Ok, here are my comments - I collected these when patch v2 was the latest but I
believe most of them are still valid.
#1
{noformat}
137 throw new IllegalArgumentException(
138 String.format("Missing %s parameter " + "(switch: %s|%s).",
139 cliOption.name, cliOption.shortSwitch,
cliOption.longSwitch));
{noformat}
{noformat}
158 if (isFile && file.isDirectory()) {
159 throw new IllegalArgumentException(String.format("Specified path
%s is a directory but should be a file " +
160 "(As value of parameter %s)", filePath, cliOption.name));
161 } else if (!isFile && !file.isDirectory()) {
162 throw new IllegalArgumentException(String.format("Specified path
%s is not a directory " +
163 "(As value of parameter %s)", filePath, cliOption.name));
164 } else if (!file.exists()) {
165 throw new IllegalArgumentException(String.format("Specified path
%s does not exist " +
166 "(As value of parameter %s)", filePath, cliOption.name));
167 }
{noformat}
These exceptions will propagate up to
{{fsConfigConversionArgumentHandler.parseAndConvert(args);}} and errors will be
printed with a stack trace.
Wouldn't it be better to use a different exception and handle it without
printing the stack trace?
#2
{noformat}
81 private enum OutputFile {
82 YARN_SITE_XML("yarn-site.xml"),
83 CAPACITY_SCHEDULER_XML("capacity-scheduler.xml");
84
85 private final String name;
86
87 OutputFile(String name) {
88 this.name = name;
89 }
90 }
{noformat}
Do we need an enum for this purpose?
#3
{noformat}
if (maxAssign != -1)
{noformat}
"-1" is a magic number, replace it with
{{FairSchedulerConfiguration.DEFAULT_MAX_ASSIGN}}.
#4
{noformat}
435 if (queueMaxAmShare != 0.0f
436 && queueMaxAmShare != queueMaxAMshareDefault
437 && queueMaxAmShare != -1.0f) {
438 capacacitySchedulerConfig.set("yarn.scheduler.capacity." +
queueName + ".maximum-am-resource-percent",
439 String.valueOf(queueMaxAmShare));
440 }
441
442 if (queueMaxAmShare == -1.0f) {
443 capacacitySchedulerConfig.set("yarn.scheduler.capacity." +
queueName + ".maximum-am-resource-percent",
444 "1.0");
445 }
{noformat}
It's my part, but direct floating point comparison could be dangerous. Let's
think this over. Basically we have to make sure that float converted from a
string "-1.0" is the same as the {{-1.0}} literal in the code.
#5
{noformat}
448 private void emitMaxRunningApps(String queueName, FSQueue queue) {
449 if (queue.getMaxRunningApps() != Integer.MAX_VALUE
{noformat}
{{Integer.MAX_VALUE}} is a magic number, extract it to a constant.
#6
{{Resources.unbounded().compareTo(...)}}
This comparison is repeated many times, should be extracted to a simple helper
method.
#7
{noformat}
681 } else if (children.size() == 1) {
682 // just a single element, probably this branch should
683 // not be reached
684 String queueName = children.get(0).getName();
685 capacities.put(queueName, Capacity.newCapacity(HUNDRED));
686 }
{noformat}
This branch is probably unnecessary. Handle {{children.size() == 0}} and
{{children.size() == 1}} cases separately before walking through the children
list.
> Migration tool that help to generate CS config based on FS config (Phase 1)
> ---------------------------------------------------------------------------
>
> Key: YARN-9699
> URL: https://issues.apache.org/jira/browse/YARN-9699
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Wanqiang Ji
> Assignee: Peter Bacsko
> Priority: Major
> Attachments: FS_to_CS_migration_POC.patch, YARN-9699-003.patch,
> YARN-9699-004.patch, YARN-9699-005.patch, YARN-9699-006.patch,
> YARN-9699-007.patch, YARN-9699-008.patch, YARN-9699-009.patch,
> YARN-9699-010.patch, YARN-9699.001.patch, YARN-9699.002.patch
>
>
--
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
