[ https://issues.apache.org/jira/browse/YARN-9699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16948795#comment-16948795 ]
Szilard Nemeth commented on YARN-9699: -------------------------------------- Hi [~pbacsko] ! Did a final round of review. Apart from fixing the checkstyle issues, I have these comments which are fixed with the patch I'm attaching now. 1. In FSConfigToCSConfigArgumentHandler#parseAndConvert: IllegalArgumentException have not caught therefore it had been resulted in an uncatched exception, up until ResourceManager.main() would caught it, but in this case, the full stacktrace would had been printed to the console. Instead, we should log the exception and show a minimum amount of information on the console (stderr). More on this in the same method, we should also catch MissingArgumentException and swallow its stacktrace, similarly to other exceptions caught in this method. As FSConfigToCSConfigArgumentHandler#parseAndConvert does not throw exceptions anymore (as it catches PreconditionException, UnsupportedPropertyException and ConversionException), the code in ResourceManager#main could not reach the System.exit(-1) call. The following piece of code could easily test the missing option vs. handling the exception properly case: {code:java} @Test public void testResourceManagerConvertFSConfigurationMissingOption() throws Exception { setupFSConfigConversionFiles(); final String mainSwitch = "-convert-fs-configuration"; FSConfigToCSConfigConverter mockConverter = mock(FSConfigToCSConfigConverter.class); ResourceManager.initFSArgumentHandler(mockConverter); ResourceManager.main(new String[] {mainSwitch}); } {code} However, I found no easy way to test if System.exit() was called with a specific value, see: https://stackoverflow.com/questions/309396/java-how-to-test-methods-that-call-system-exit Moreover, if System.exit is called, the JUnit testcase will fail so I'd refrain to add a new testcase for this, which I don't like at all but this is what it is. 2. Nit: FSYarnSiteConverter: Typo in javadoc of this class. "Converts a Fair Schedule". Last word should be "scheduler". 3. Nit: QueuePlacementConverter#convertPlacementPolicy: rulehandler parameter should be camelcase. 4. QueuePlacementConverter: "u:%user:%user" is occuring twice, it should be extracted to a constant. I'd also extract %user, %primary_group and %secondary_group to constants. 5. QueuePlacementConverter: if-else chain is hard to read. Please extract at least the nested rule handling part to a separate method. 6. Nit: FSConfigToCSConfigConverter: There are a bunch of occasions of strings: "yarn-site.xml" / "capacity-scheduler.xml" and we have constants for them. We should use the constants wherever possible. 7. Nit: FSConfigToCSConfigConverter#prepareOutputFiles: Inconsistent naming: yarnSiteXmlOutputFile / schedulerXmlOutput. This can confuse readers. 8. Nit: Name of method FSConfigToCSConfigConverter#emitDefaultMaxAMshare: Share should be with capital S. 9. FSConfigToCSConfigConverter#convert receives the cluster resource as parameter but this method only sets it to a field and does nothing else with it: The caller code should simply set the field and not pass the resource as a parameter. 10. Nit: In FSConfigToCSConfigConverter#convertCapacitySchedulerXml: properties.entrySet().forEach could be replaced with properties.forEach. 11. Nit: In FSConfigToCSConfigConverter#emitACLs: entrySet().stream().forEach can be replaced with entrySet().forEach 12. Nit: In FSConfigToCSConfigRuleHandler#loadRulesFromFile: rules variable could be inlined. 13. Nit: FSQueueConverter#MAXRUNNINGAPPS_UNSET should be renamed to MAX_RUNNING_APPS_UNSET. 14. In FSQueueConverter#convertQueueHierarchy: Please move comments above method calls to their javadoc instead. 15. Nit: In FSQueueConverter#emitChildQueues: Call to toString() is unnecessary in statement: capacitySchedulerConfig.set(PREFIX + queueName + ".queues", childQueues.toString()); 16. Nit: FSQueueConverter#emitChildCapacity: entrySet().stream().forEach can be replaced with entrySet().forEach I fixed the following issues above and uploaded a (hopefully) final patch. Waiting for jenkins results... In the meantime, [~sunilg] : Do you have some free cycles to review this patch? If not, I will commit the latest patch EOD tomorrow. Thanks! > [Phase 1] Migration tool that help to generate CS config based on FS config > --------------------------------------------------------------------------- > > 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-011.patch, YARN-9699-012.patch, > YARN-9699-013.patch, YARN-9699-014.patch, YARN-9699-015.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