[
https://issues.apache.org/jira/browse/YARN-10067?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17009892#comment-17009892
]
Szilard Nemeth commented on YARN-10067:
---------------------------------------
Hi [~pbacsko]!
Thanks for your patch. I have some comments:
1. In FSConfigToCSConfigArgumentHandler#parseAndConvert: Please extract the
code from
{code:java}
dryRun = cliParser.hasOption(CliOption.DRY_RUN.shortSwitch);
{code}
until
{code:java}
converter.convert(params);
{code}
to a separate method, for better readability.
2. In FSConfigToCSConfigArgumentHandler#parseAndConvert: The exception handling
logic is quite verbose as of now.
You could extract a code block that is occurring 3 times of the exception
handling:
{code:java}
if (dryRun) {
dryRunResultHolder.addDryRunError(msg);
} else {
logAndStdErr(e, msg);
return -1;
}
{code}
3. I think FSConfigToCSConfigArgumentHandler#printDryRunResults is a good
method, in terms of contents. I would rather move the whole printing logic into
DryRunResultHolder instead, so printing its own results can be the
responsibility of that class.
4. Nit: FSConfigToCSConfigConverter#dryRun: You can omit "= false" from the
declaration, since as per Java standards, booleans are initialized to false by
default.
5. I can see in many places that the boolean dryRun + the DryRunResultsHolder
are passed in tandem. For example, in FSConfigToCSConfigConverter, in
FSConfigToCSConfigRuleHandler and in FSConfigToCSConfigArgumentHandler.
Can you create a class to hold these two together?
For example, I can image something named like "RuntimeParameters" and there you
could hide the details like dry run, as well as any other future runtime
options.
Methods like FSConfigToCSConfigRuleHandler#handle and
FSQueueConverter#convertQueueHierarchy could simply pass (delegate) the
exceptionMessage to an instance of this RuntimeParameters class and the
instance could decide on what to do with the error message:
Either throw an UnsupportedPropertyException or to record it as a dry run
error. This way, the dry-run feature is better abstracted, in my opinion.
6. Why don't you use FSConfigToCSConfigConverterParams#isDryRun anywhere? Is
this intentional?
7. In TestFSQueueConverter, you have very similar code calls to create the
FSQueueConverter objects. I would suggest to extract a method that creates a
builder object with those common calls, e.g.
{code:java}
FSQueueConverterBuilder.create()
.withRuleHandler(ruleHandler)
.withCapacitySchedulerConfig(csConfig)
.withPreemptionEnabled(false)
.withSizeBasedWeight(false)
.withAutoCreateChildQueues(true)
.withClusterResource(CLUSTER_RESOURCE)
.withQueueMaxAMShareDefault(0.16f)
.withQueueMaxAppsDefault(15)
.withDryRun(false)
{code}
and then tweak the builder to meet the testcase needs. This way, you
can have a default builder object with a few additional calls to it to prepare
the converter object.
> Add dry-run feature to FS-CS converter tool
> -------------------------------------------
>
> Key: YARN-10067
> URL: https://issues.apache.org/jira/browse/YARN-10067
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Peter Bacsko
> Assignee: Peter Bacsko
> Priority: Major
> Attachments: YARN-10067-001.patch, YARN-10067-002.patch,
> YARN-10067-003.patch
>
>
> Add a "d" / "-dry-run" switch to the tool. The purpose of this would be to
> inform the user whether a conversion is possible and if it is, are there any
> warnings.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
