abstractdog commented on code in PR #4630:
URL: https://github.com/apache/hive/pull/4630#discussion_r1306939300
##########
standalone-metastore/metastore-common/src/test/java/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java:
##########
@@ -74,7 +74,9 @@ public static void setUp() throws Exception {
conf.set(MetastoreConf.ConfVars.METRICS_CLASS.getHiveName(),
CodahaleMetrics.class.getCanonicalName());
conf.set(MetastoreConf.ConfVars.HIVE_CODAHALE_METRICS_REPORTER_CLASSES.getHiveName(),
"org.apache.hadoop.hive.common.metrics.metrics2.JsonFileMetricsReporter, "
- +
"org.apache.hadoop.hive.common.metrics.metrics2.JmxMetricsReporter");
+ +
"org.apache.hadoop.hive.common.metrics.metrics2.JmxMetricsReporter, "
+ +
"org.apache.hadoop.hive.common.metrics.metrics2.Metrics2Reporter, "
Review Comment:
please create a separate unit test for this, it's quite confusing that we
intentionally add configs to a unit test setup in a bad way to confirm
everything is working fine: a unit test should be explicit about what to do and
why
around the new unit test (and the code fix), it should be clarified that:
a) in what circumstances the reporter can be added twice (+who adds that?)
b) how is the patch fixing that?
c) why fixing it here (by a shutdown) is better than making sure that it's
not added twice somewhere else
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
