Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/315#discussion_r206597366
  
    --- Diff: 
phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixMetricsIT.java ---
    @@ -204,6 +230,45 @@ private static void resetGlobalMetrics() {
             }
         }
     
    +    // Phoenix Client Metrics are transported via Hadoop-metrics2 sink
    +    // The test sink is defined at GlobalPhoenixMetricsTestSink
    +    // Configuration for Hadoop-metrics2 comes from 
hadoop-metrics2.properties file located in test/resources
    +    private boolean verifyMetricsFromSink() throws InterruptedException {
    +        Map<String, Long> expectedMetrics = new HashMap<>();
    +        for (GlobalMetric m : 
PhoenixRuntime.getGlobalPhoenixClientMetrics()) {
    +            expectedMetrics.put(m.getMetricType().name(), m.getTotalSum());
    +        }
    +
    +        for (int i = 0; i < MAX_RETRIES; i++) {
    +            LOG.info("Verifying Global Metrics from Hadoop Sink, Retry: " 
+ (i + 1));
    +            if (verifyMetricsFromSinkOnce(expectedMetrics)) {
    +                LOG.info("Values from Hadoop Metrics Sink match actual 
values");
    +                return true;
    +            }
    +            Thread.sleep(1000);
    +        }
    +        return false;
    +    }
    +
    +    private boolean verifyMetricsFromSinkOnce(Map<String, Long> 
expectedMetrics) {
    +        synchronized (GlobalPhoenixMetricsTestSink.lock) {
    +            for (AbstractMetric metric : 
GlobalPhoenixMetricsTestSink.metrics) {
    +                if (expectedMetrics.containsKey(metric.name())) {
    --- End diff --
    
    I thought there was an issue with unboxing the value from 
`expectedMetrics.get(metric.name())`.
    
    I think it would be cleaner to do:
    ```
    Long value = expectedMetrics.get(metric.name());
    if (value != null) {
      long expectedValue = value.longValue();
      ...
    ```


---

Reply via email to