[
https://issues.apache.org/jira/browse/YARN-9699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16948795#comment-16948795
]
Szilard Nemeth edited comment on YARN-9699 at 10/10/19 5:13 PM:
----------------------------------------------------------------
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...
[~pbacsko] : Please review the changes between patch15 and patch16!
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!
was (Author: snemeth):
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-016.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
