azkaban-aplcache

Fix flaky metrics related tests (#1018) Instead of using

4/25/2017 2:44:46 PM

Details

diff --git a/azkaban-common/src/test/java/azkaban/metrics/CommonMetricsTest.java b/azkaban-common/src/test/java/azkaban/metrics/CommonMetricsTest.java
index 4c6287d..75ffc5a 100644
--- a/azkaban-common/src/test/java/azkaban/metrics/CommonMetricsTest.java
+++ b/azkaban-common/src/test/java/azkaban/metrics/CommonMetricsTest.java
@@ -16,48 +16,40 @@
 
 package azkaban.metrics;
 
-import azkaban.metrics.MetricsTestUtility.DummyReporter;
-
-import java.util.concurrent.TimeUnit;
-import java.time.Duration;
-
-import org.junit.After;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 
-@Ignore
-//todo: HappyRay Find a way to fix these flaky tests.
-public class CommonMetricsTest {
-
-  private DummyReporter dr;
-
-  @Before
-  public void setup() {
-    dr = new DummyReporter(MetricsManager.INSTANCE.getRegistry());
-    dr.start(Duration.ofMillis(2).toMillis(), TimeUnit.MILLISECONDS);
-  }
+import static org.junit.Assert.*;
 
-  @After
-  public void shutdown() {
-    if (null != dr)
-      dr.stop();
 
-    dr = null;
-  }
+public class CommonMetricsTest {
+  private MetricsTestUtility testUtil;
+  private CommonMetrics metrics;
 
-  @Test
-  public void testMarkDBConnectionMetrics() {
-    MetricsTestUtility.testMeter("DB-Connection-meter", dr, CommonMetrics.INSTANCE::markDBConnection);
+  @Before
+  public void setUp() {
+    // Use of global state can cause problems e.g.
+    // The state is shared among tests.
+    // e.g. we can't run a variant of the testOOMWaitingJobMetrics twice since it relies on the initial state of
+    // the registry.
+    // This can also cause problem when we run tests in parallel in the future.
+    // todo HappyRay: move MetricsManager, CommonMetrics to use Juice.
+    testUtil = new MetricsTestUtility(MetricsManager.INSTANCE.getRegistry());
+    metrics = CommonMetrics.INSTANCE;
   }
 
   @Test
   public void testDBConnectionTimeMetrics() {
-    MetricsTestUtility.testGauge("dbConnectionTime", dr, CommonMetrics.INSTANCE::setDBConnectionTime);
+    metrics.setDBConnectionTime(14);
+    assertEquals(14, testUtil.getGaugeValue("dbConnectionTime"));
   }
 
   @Test
   public void testOOMWaitingJobMetrics() {
-    MetricsTestUtility.testGauge("OOM-waiting-job-count", dr, CommonMetrics.INSTANCE::incrementOOMJobWaitCount);
+    final String metricName = "OOM-waiting-job-count";
+
+    assertEquals(0, testUtil.getGaugeValue(metricName));
+    metrics.incrementOOMJobWaitCount();
+    assertEquals(1, testUtil.getGaugeValue(metricName));
   }
 }
diff --git a/azkaban-common/src/test/java/azkaban/metrics/MetricsTestUtility.java b/azkaban-common/src/test/java/azkaban/metrics/MetricsTestUtility.java
index 063581e..545b0af 100644
--- a/azkaban-common/src/test/java/azkaban/metrics/MetricsTestUtility.java
+++ b/azkaban-common/src/test/java/azkaban/metrics/MetricsTestUtility.java
@@ -16,116 +16,24 @@
 
 package azkaban.metrics;
 
-import com.codahale.metrics.Counter;
-import com.codahale.metrics.Gauge;
-import com.codahale.metrics.Histogram;
-import com.codahale.metrics.Meter;
-import com.codahale.metrics.MetricFilter;
 import com.codahale.metrics.MetricRegistry;
-import com.codahale.metrics.ScheduledReporter;
-import com.codahale.metrics.Timer;
 
-import org.junit.Assert;
-
-import java.util.HashMap;
-import java.util.Map;
-import java.util.SortedMap;
-import java.util.concurrent.TimeUnit;
-import java.util.function.Consumer;
 
 /**
  * This class is designed for a utility class to test drop wizard metrics
  */
 public class MetricsTestUtility {
 
-  /**
-   * One Dummy Reporter extending {@link ScheduledReporter} collects metrics in various maps,
-   * which test methods are able to access easily.
-   */
-  public static class DummyReporter extends ScheduledReporter{
-
-    private Map<String, String> gaugeMap;
-
-    private Map<String, Long> meterMap;
-
-    public DummyReporter(MetricRegistry registry) {
-      super(registry, "dummy-reporter", MetricFilter.ALL, TimeUnit.SECONDS, TimeUnit.MILLISECONDS);
-      this.gaugeMap = new HashMap<>();
-      this.meterMap = new HashMap<>();
-    }
-
-    @Override
-    public void report(SortedMap<String, Gauge> gauges,
-        SortedMap<String, Counter> counters,
-        SortedMap<String, Histogram> histograms,
-        SortedMap<String, Meter> meters,
-        SortedMap<String, Timer> timers) {
+  // todo HappyRay: move singletons to Juice.
+  // This can cause problems when we run tests in parallel in the future.
+  private MetricRegistry registry;
 
-      for (Map.Entry<String, Gauge> entry : gauges.entrySet()) {
-        gaugeMap.put(entry.getKey(), entry.getValue().getValue().toString());
-      }
-
-      for (Map.Entry<String, Meter> entry : meters.entrySet()) {
-        meterMap.put(entry.getKey(), entry.getValue().getCount());
-      }
-    }
-
-    private String getGauge(String key) {
-      return gaugeMap.get(key);
-    }
-
-    private long getMeter(String key) {
-      return meterMap.getOrDefault(key, 0L);
-    }
+  public MetricsTestUtility(MetricRegistry registry) {
+    this.registry = registry;
   }
 
-  public static void testMeter(String meterName, DummyReporter dr, Runnable runnable) {
-
-    sleepMillis(20);
-    long currMeter = dr.getMeter(meterName);
-    runnable.run();
-    sleepMillis(20);
-    Assert.assertEquals(dr.getMeter(meterName), currMeter + 1);
-
-    runnable.run();
-    runnable.run();
-    sleepMillis(20);
-    Assert.assertEquals(dr.getMeter(meterName), currMeter + 3);
-  }
-
-  public static void testGauge(String meterName, DummyReporter dr, Runnable runnable) {
-    sleepMillis(20);
-    long currMeter = Long.valueOf(dr.getGauge(meterName));
-    runnable.run();
-    sleepMillis(20);
-    Assert.assertEquals(Long.valueOf(dr.getGauge(meterName)).longValue(), currMeter + 1);
-
-    runnable.run();
-    runnable.run();
-    sleepMillis(20);
-    Assert.assertEquals(Long.valueOf(dr.getGauge(meterName)).longValue(), currMeter + 3);
-  }
-
-  public static void testGauge(String GaugeName, DummyReporter dr, Consumer<Long> func) {
-    func.accept(1L);
-
-    // needs time to let metrics reporter receive the updated value.
-    sleepMillis(50);
-    Assert.assertEquals(dr.getGauge(GaugeName), "1");
-    func.accept(99L);
-    sleepMillis(50);
-    Assert.assertEquals(dr.getGauge(GaugeName), "99");
-  }
-
-
-  /**
-   * Helper method to sleep milli seconds.
-   */
-  private static void sleepMillis(int numMilli) {
-    try {
-      Thread.sleep(numMilli);
-    } catch (InterruptedException e) {
-      e.printStackTrace();
-    }
+  public long getGaugeValue(String name) {
+    // Assume that the gauge value can be converted to type long.
+    return (long) registry.getGauges().get(name).getValue();
   }
 }
diff --git a/azkaban-web-server/src/test/java/azkaban/webapp/WebMetricsTest.java b/azkaban-web-server/src/test/java/azkaban/webapp/WebMetricsTest.java
index 46a7cb3..c668ec7 100644
--- a/azkaban-web-server/src/test/java/azkaban/webapp/WebMetricsTest.java
+++ b/azkaban-web-server/src/test/java/azkaban/webapp/WebMetricsTest.java
@@ -17,47 +17,28 @@
 package azkaban.webapp;
 
 import azkaban.metrics.MetricsManager;
-import azkaban.metrics.MetricsTestUtility.DummyReporter;
 import azkaban.metrics.MetricsTestUtility;
-
-import java.time.Duration;
-import java.util.concurrent.TimeUnit;
-
-import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
+import static org.junit.Assert.*;
 
-public class WebMetricsTest{
-
-  private DummyReporter dr;
 
-  @Before
-  public void setup() {
-    dr = new DummyReporter(MetricsManager.INSTANCE.getRegistry());
-    dr.start(Duration.ofMillis(2).toMillis(), TimeUnit.MILLISECONDS);
-  }
+public class WebMetricsTest {
 
-  @After
-  public void shutdown() {
-    if (null != dr)
-      dr.stop();
+  private MetricsTestUtility testUtil;
+  private WebMetrics metrics;
 
-    dr = null;
+  @Before
+  public void setUp() {
+    // todo HappyRay: move MetricsManager, WebMetrics to use Juice.
+    testUtil = new MetricsTestUtility(MetricsManager.INSTANCE.getRegistry());
+    metrics = WebMetrics.INSTANCE;
   }
 
   @Test
   public void testLogFetchLatencyMetrics() {
-    MetricsTestUtility.testGauge("fetchLogLatency", dr, WebMetrics.INSTANCE::setFetchLogLatency);
-  }
-
-  @Test
-  public void testWebPostCallMeter() {
-    MetricsTestUtility.testMeter("Web-Post-Call-Meter", dr, WebMetrics.INSTANCE::markWebPostCall);
-  }
-
-  @Test
-  public void testWebGetCallMeter() {
-    MetricsTestUtility.testMeter("Web-Get-Call-Meter", dr, WebMetrics.INSTANCE::markWebGetCall);
+    metrics.setFetchLogLatency(14);
+    assertEquals(14, testUtil.getGaugeValue("fetchLogLatency"));
   }
 }