hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Szilard Nemeth (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (YARN-8750) Refactor TestQueueMetrics
Date Mon, 01 Oct 2018 22:14:00 GMT

    [ 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: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message