[
https://issues.apache.org/jira/browse/YARN-9699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16944579#comment-16944579
]
Szilard Nemeth edited comment on YARN-9699 at 10/4/19 3:08 PM:
---------------------------------------------------------------
Hi [~pbacsko]!
Thanks for the patch, good work!
Here are my comments:
1. Typo on name of field: FSConfigToCSConfigConverter#capacacitySchedulerConfig
2. Typo on name of field: FSConfigToCSConfigConverter#capacacitySchedulerConfig
3. Name FSConfigToCSConfigConverter#.queueMaxAMshareDefault could be
queueMaxAMShareDefault (capital S in Share). Name of method
FSConfigToCSConfigConverter#emitMaxAMshare is having a similar issue.
4. You have an unused field FSConfigToCSConfigConverter#placementManager. Most
probably it's not needed as you have a variable with the same name in the
convert method.
5. Cluster resource is only passed as a parameter to convert
FSConfigToCSConfigConverter#convert(org.apache.hadoop.conf.Configuration,
org.apache.hadoop.yarn.api.records.Resource) to be able to set it as a field
value.
6. In class FSConfigToCSConfigConverter: You have 2 comments, separating the
code generation of yarn-site.xml / capacity-scheduler.xml (// conversion --
yarn-site.xml). I would introduce 2 methods instead.
7. In class FSConfigToCSConfigConverter: You have 2 unnecessary String.valueOf
calls:
yarnSiteConfig.set("yarn.scheduler.minimum-allocation-mb",
String.valueOf(mbIncrementAllocation));
yarnSiteConfig.set("yarn.scheduler.minimum-allocation-vcores",
String.valueOf(vcoreIncrementAllocation));
8. You have many methods that are only invoked from
FSConfigToCSConfigConverter#convertQueue named emitXXX. Please create a new
class that can convert a queue, making the converter class smaller. Also, we
can treat converting a queue as a separate concern.
9. In FSConfigToCSConfigConverter#getQueueShortName: Second parameter to
subString call is redundant.
10. FSConfigToCSConfigConverter: You could extract "yarn.scheduler.capacity."
to a constant at it is heavily used across the class.
11. FSConfigToCSConfigConverter#getCapacities: You have
String capacity = String.format("[memory=%d,vcores=%d]",
minShare.getMemorySize(), minShare.getVirtualCores());
This ignores resource types. I can accept that in Phase 1, but please mark it
in the code that we need to take care of it later.
12. FSConfigToCSConfigConverter#checkMaxChildCapacitySetting: queueName
parameter is unused.
13. Typo in the name of method:
FSConfigToCSConfigRuleHandler#handleSecifiedNotFirstRule
14. In method QueuePlacementConverter#convertPlacementPolicy: You have
instanceof conditions, but the conditions are following each other and they are
not connected as else-if statements. Please use else-if instead!
15. My mistake: ResourceManager#printUsage: Console parameter is not -c
anymore. Also, the resource parameter is not listed. This help text must be
in-line with the enum values here: FSConfigToCSConfigArgumentHandler.CliOption
16. General comment for TestFSConfigToCSConfigConverter: Please use assertJ
methods (assertThat() calls) instead of JUnit assert. Please note that this is
not a must-have, just wanted to make sure the class is consistent. Moreover,
messages from assertJ are more informative. You can see a similar initiative
has been started by @adam.antal with HADOOP-16509.
17. General comment for TestFSConfigToCSConfigConverter: Can we separate the
yarn-site.xml / fair-scheduler.xml conversions both in production and test
code?
18. Two calls of assertEquals in
TestFSConfigToCSConfigConverter#testQueuePreemptionDisabled could be assertTrue
or the respective assertJ method.
You have one similar in these methods:
* testSiteContinuousSchedulingConversion
* testSitePreemptionConversion
* testSiteAssignMultipleConversion
was (Author: snemeth):
Hi [~pbacsko]!
Thanks for the patch, good work!
Here are my comments:
1. Typo on name of field: FSConfigToCSConfigConverter#capacacitySchedulerConfig
2. Typo on name of field: FSConfigToCSConfigConverter#capacacitySchedulerConfig
3. Name FSConfigToCSConfigConverter#.queueMaxAMshareDefault could be
queueMaxAMShareDefault (capital S in Share). Name of method
FSConfigToCSConfigConverter#emitMaxAMshare is having a similar issue.
4. You have an unused field FSConfigToCSConfigConverter#placementManager. Most
probably it's not needed as you have a variable with the same name in the
convert method.
5. Cluster resource is only passed as a parameter to convert
FSConfigToCSConfigConverter#convert(org.apache.hadoop.conf.Configuration,
org.apache.hadoop.yarn.api.records.Resource) to be able to set it as a field
value.
6. In class FSConfigToCSConfigConverter: You have 2 comments, separating the
code generation of yarn-site.xml / capacity-scheduler.xml (// conversion --
yarn-site.xml). I would introduce 2 methods instead.
7. In class FSConfigToCSConfigConverter: You have 2 unnecessary String.valueOf
calls:
yarnSiteConfig.set("yarn.scheduler.minimum-allocation-mb",
String.valueOf(mbIncrementAllocation));
yarnSiteConfig.set("yarn.scheduler.minimum-allocation-vcores",
String.valueOf(vcoreIncrementAllocation));
8. You have many methods that are only invoked from
FSConfigToCSConfigConverter#convertQueue named emitXXX. Please create a new
class that can convert a queue, making the converter class smaller. Also, we
can treat converting a queue as a separate concern.
9. In FSConfigToCSConfigConverter#getQueueShortName: Second parameter to
subString call is redundant.
10. FSConfigToCSConfigConverter: You could extract "yarn.scheduler.capacity."
to a constant at it is heavily used across the class.
11. FSConfigToCSConfigConverter#getCapacities: You have
String capacity = String.format("[memory=%d,vcores=%d]",
minShare.getMemorySize(), minShare.getVirtualCores());
This ignores resource types. I can accept that in Phase 1, but please mark it
in the code that we need to take care of it later.
12. FSConfigToCSConfigConverter#checkMaxChildCapacitySetting: queueName
parameter is unused.
13. Typo in the name of method:
FSConfigToCSConfigRuleHandler#handleSecifiedNotFirstRule
14. In method QueuePlacementConverter#convertPlacementPolicy: You have
instanceof conditions, but the conditions are following each other and they are
not connected as else-if statements. Please use else-if instead!
15. My mistake: ResourceManager#printUsage: Console parameter is not -c
anymore. Also, the resource parameter is not listed. This help text must be
in-line with the enum values here: FSConfigToCSConfigArgumentHandler.CliOption
16. General comment for TestFSConfigToCSConfigConverter: Please use assertJ
methods (assertThat() calls) instead of JUnit assert. Please note that this is
not a must-have, just wanted to make sure the class is consistent. Moreover,
messages from assertJ are more informative. You can see a similar initiative
has been started by @adam.antal with HADOOP-16509.
17. Two calls of assertEquals in
TestFSConfigToCSConfigConverter#testQueuePreemptionDisabled could be assertTrue
or the respective assertJ method.
You have one similar in these methods:
* testSiteContinuousSchedulingConversion
* testSitePreemptionConversion
* testSiteAssignMultipleConversion
18. General comment for TestFSConfigToCSConfigConverter: Can we separate the
yarn-site.xml / fair-scheduler.xml conversions both in production and test
code?
> 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.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
