[ 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