[ https://issues.apache.org/jira/browse/YARN-10130?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17040101#comment-17040101 ]
Szilard Nemeth edited comment on YARN-10130 at 2/19/20 2:36 PM: ---------------------------------------------------------------- Hi [~adam.antal], Thanks for this patch. Some comments: 1. Nit: FSConfigToCSConfigArgumentHandler#checkOutputDirDoesNotContainsXmls: Should be renamed to "checkOutputDirDoesNotContainXml" 2. Would FSConfigToCSConfigArgumentHandler#checkOutputDirDoesNotContainsXmls work correctly if the String "yarnSiteXmlFile" contains a relative name only? I mean if -y yarn-site.xml is provided, I'm not sure if the following code block will work correctly: {code} File xmlFile = new File(yarnSiteXmlFile); File xmlParentFolder = xmlFile.getParentFile(); {code} In this case, I'm not sure about the value of xmlParentFolder, but maybe I'm wrong. 3. In FSConfigToCSConfigArgumentHandler#checkOutputDirDoesNotContainsXmls, when you throw the IllegalArgumentException, you should use yet another static final string and not the string literal as you used static string for a different case in checkFileNotInOutputDir. This way, the code could be more consistent. 4. Nit: Comment: {code:java} // check whether the output folder does not contain not yarn-site.xml // neither capacity-scheduler.xml {code} Should be: "... nor yarn-site.xml neither..." 5. In checkFileNotInOutputDir: Variable name xmlChild is a bit weird :) 6. In general, I don't get what's going on: You first call checkOutputDirDoesNotContainsXmls and the first code block checks if yarn-site.xml can be found in the output directory. Then you call checkFileNotInOutputDir, once with YARN_SITE_CONFIGURATION_FILE, then with CS_CONFIGURATION_FILE. Aren't you checking yarn-site.xml twice, unnecessarily? Consequently, I don't get what this testcase does: TestFSConfigToCSConfigArgumentHandler#testYarnSiteOptionInOutputFolder I think the other 2 testcases are enough. 7. I think the code that throws the exception in checkFileNotInOutputDir has parameters in a wrong order: 1st param should be: CliOption.OUTPUT_DIR.name 2nd param should be: output was (Author: snemeth): Hi [~adam.antal], Thanks for this patch. Some comments: 1. Nit: FSConfigToCSConfigArgumentHandler#checkOutputDirDoesNotContainsXmls: Should be renamed to "checkOutputDirDoesNotContainXml" 2. Would FSConfigToCSConfigArgumentHandler#checkOutputDirDoesNotContainsXmls work correctly if the String "yarnSiteXmlFile" contains a relative name only? I mean if -y yarn-site.xml is provided, I'm not sure if the following code block will work correctly: {code} File xmlFile = new File(yarnSiteXmlFile); File xmlParentFolder = xmlFile.getParentFile(); {code} In this case, I'm not sure about the value of xmlParentFolder, but maybe I'm wrong. 3. In FSConfigToCSConfigArgumentHandler#checkOutputDirDoesNotContainsXmls, when you throw the IllegalArgumentException, you should use yet another static final string and not the string literal as you used static string for a different case in checkFileNotInOutputDir. This way, the code could be more consistent. 4. Nit: Comment: // check whether the output folder does not contain not yarn-site.xml // neither capacity-scheduler.xml Should be: "... nor yarn-site.xml neither..." 5. In checkFileNotInOutputDir: Variable name xmlChild is a bit weird :) 6. In general, I don't get what's going on: You first call checkOutputDirDoesNotContainsXmls and the first code block checks if yarn-site.xml can be found in the output directory. Then you call checkFileNotInOutputDir, once with YARN_SITE_CONFIGURATION_FILE, then with CS_CONFIGURATION_FILE. Aren't you checking yarn-site.xml twice, unnecessarily? Consequently, I don't get what this testcase does: TestFSConfigToCSConfigArgumentHandler#testYarnSiteOptionInOutputFolder I think the other 2 testcases are enough. 7. I think the code that throws the exception in checkFileNotInOutputDir has parameters in a wrong order: 1st param should be: CliOption.OUTPUT_DIR.name 2nd param should be: output > FS-CS converter: Do not allow output dir to be the same as input dir > -------------------------------------------------------------------- > > Key: YARN-10130 > URL: https://issues.apache.org/jira/browse/YARN-10130 > Project: Hadoop YARN > Issue Type: Sub-task > Reporter: Szilard Nemeth > Assignee: Adam Antal > Priority: Major > Attachments: YARN-10130.001.patch, YARN-10130.002.patch, > YARN-10130.003.patch > > > If the input dir where fair-scheduler.xml / yarn-site.xml sits is the same as > the output dir (defined by the -o switch), the fs2cs tool overwrites the > source config files, i.e. yarn-site.xml. > Reproduce this is easy, just run fs2cs tool with this command: > {code:java} > /bin/yarn fs2cs --cluster-resource memory-mb=18044928,vcores=16 > --no-terminal-rule-check -y yarn-site.xml -f fair-scheduler.xml -o . > {code} > The following (or similar) is emitted by the tool: > {code:java} > WARNING: YARN_OPTS has been replaced by HADOOP_OPTS. Using value of > YARN_OPTS.WARNING: YARN_OPTS has been replaced by HADOOP_OPTS. Using value of > YARN_OPTS.20/02/10 12:51:42 INFO converter.FSConfigToCSConfigConverter: > Output directory for yarn-site.xml and capacity-scheduler.xml is: .20/02/10 > 12:51:42 INFO converter.FSConfigToCSConfigConverter: Conversion rules file is > not defined, using default conversion config!20/02/10 12:51:42 ERROR > conf.Configuration: error parsing conf > yarn-site.xmlcom.ctc.wstx.exc.WstxEOFException: Unexpected EOF in prolog at > [row,col,system-id]: [1,0,"yarn-site.xml"] at > com.ctc.wstx.sr.StreamScanner.throwUnexpectedEOF(StreamScanner.java:687) at > com.ctc.wstx.sr.BasicStreamReader.handleEOF(BasicStreamReader.java:2220) at > com.ctc.wstx.sr.BasicStreamReader.nextFromProlog(BasicStreamReader.java:2126) > at com.ctc.wstx.sr.BasicStreamReader.next(BasicStreamReader.java:1181) at > org.apache.hadoop.conf.Configuration$Parser.parseNext(Configuration.java:3343) > at > org.apache.hadoop.conf.Configuration$Parser.parse(Configuration.java:3137) at > org.apache.hadoop.conf.Configuration.loadResource(Configuration.java:3030) at > org.apache.hadoop.conf.Configuration.loadResources(Configuration.java:2996) > at org.apache.hadoop.conf.Configuration.getProps(Configuration.java:2871) at > org.apache.hadoop.conf.Configuration.set(Configuration.java:1389) at > org.apache.hadoop.conf.Configuration.set(Configuration.java:1361) at > org.apache.hadoop.conf.Configuration.setBoolean(Configuration.java:1702) at > org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.converter.FSConfigToCSConfigConverter.createConfiguration(FSConfigToCSConfigConverter.java:166) > at > org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.converter.FSConfigToCSConfigConverter.convert(FSConfigToCSConfigConverter.java:98) > at > org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.converter.FSConfigToCSConfigArgumentHandler.parseAndConvert(FSConfigToCSConfigArgumentHandler.java:137) > at > org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.converter.FSConfigToCSConfigConverterMain.main(FSConfigToCSConfigConverterMain.java:40)20/02/10 > 12:51:42 ERROR converter.FSConfigToCSConfigConverterMain: Error while > starting FS configuration conversion! > {code} -- 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