[
https://issues.apache.org/jira/browse/YARN-8750?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16634691#comment-16634691
]
Szilard Nemeth edited comment on YARN-8750 at 10/1/18 10:13 PM:
----------------------------------------------------------------
The changes in TestQueueMetrics could have been more simple if I had used a
map, but using a separate "checker" class for verification is having some
advantages that are not visible in the first place:
1. The possiblity of accidentally interchange Resource metrics and App metrics
assertions are enforced, there are dedicated checker classes for those along
with their respective enums. For example, {{AppMetricsChecker}} only accepts
{{AppMetricsKey}} s. The same goes with {{ResourceMetricsChecker}} and
{{ResourceMetricsKey}} s.
2. The mentioned enums guarantee that only existing resource metrics / app
metrics keys are used in tests.
3. The methods named as {{checkAll}} in the two checker classes are hiding the
complexity of asserting gauge and counter values. As the functionality of
{{checkAll}} could be replaced with 3 maps in every test classes where the test
want to verify metrics, this could lead to unnecessary code duplication, so the
current solution is more reusable.
4. Methods {{gaugeLong}}, {{gaugeInt}} and {{counter}} in
{{ResourceMetricsChecker}} put the values in the correct map. If the tests
themselves were referencing those maps, it would be easier to put the value to
a wrong map unintentionally.
I'm open to rename the {{checkAll}} method as one can come up with a better
name, but that's what I got for now.
was (Author: snemeth):
The changes in TestQueueMetrics could have been more simple if I had used a
map, but using a separate "checker" class for verification is having some
advantages that are not visible in the first place:
1. The possiblity of accidentally interchange Resource metrics and App metrics
assertions are enforced, there are dedicated checker classes for those along
with their respective enums. For example, {{AppMetricsChecker}} only accepts
{{AppMetricsKey}}s. The same goes with {{ResourceMetricsChecker}} and
{{ResourceMetricsKey}}s.
2. The mentioned enums guarantee that only existing resource metrics / app
metrics keys are used in tests.
3. The methods named as {{checkAll}} in the two checker classes are hiding the
complexity of asserting gauge and counter values. As the functionality of
{{checkAll}} could be replaced with 3 maps in every test classes where the test
want to verify metrics, this could lead to unnecessary code duplication, so the
current solution is more reusable.
4. Methods {{gaugeLong}}, {{gaugeInt}} and {{counter}} in
{{ResourceMetricsChecker}} put the values in the correct map. If the tests
themselves were referencing those maps, it would be easier to put the value to
a wrong map unintentionally.
I'm open to rename the {{checkAll}} method as one can come up with a better
name, but that's what I got for now.
> Refactor TestQueueMetrics
> -------------------------
>
> Key: YARN-8750
> URL: https://issues.apache.org/jira/browse/YARN-8750
> Project: Hadoop YARN
> Issue Type: Improvement
> Components: resourcemanager
> Reporter: Szilard Nemeth
> Assignee: Szilard Nemeth
> Priority: Minor
> Attachments: YARN-8750.001.patch, YARN-8750.002.patch,
> YARN-8750.003.patch
>
>
> {{TestQueueMetrics#checkApps}} and {{TestQueueMetrics#checkResources}} have 8
> and 14 parameters, respectively.
> It is very hard to read the testcases that are using these methods.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
