HADOOP-11361. Fix a race condition in MetricsSourceAdapter.updateJmxC…

…ache. Contributed by Vinayakumar B, Yongjun Zhang, and Brahma Reddy Battula. (ozawa)
serhiy · Jul 13, 2016 · 77ffe76 · 77ffe76
1 parent 438b7c5
commit 77ffe76
Showing 1 changed file with 14 additions and 19 deletions.
diff --git a/...ect/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java b/...ect/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java
@@ -31,6 +31,7 @@
 
 import static com.google.common.base.Preconditions.*;
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.Maps;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -59,7 +60,6 @@ class MetricsSourceAdapter implements DynamicMBean {
   private final MBeanInfoBuilder infoBuilder;
   private final Iterable<MetricsTag> injectedTags;
 
-  private Iterable<MetricsRecordImpl> lastRecs;
   private boolean lastRecsCleared;
   private long jmxCacheTS = 0;
   private long jmxCacheTTL;
@@ -175,30 +175,26 @@ private void updateJmxCache() {
       }
     }
 
+    // HADOOP-11361: Release lock here for avoid deadlock between
+    // MetricsSystemImpl's lock and MetricsSourceAdapter's lock.
+    Iterable<MetricsRecordImpl> lastRecs = null;
     if (getAllMetrics) {
-      MetricsCollectorImpl builder = new MetricsCollectorImpl();
-      getMetrics(builder, true);
+      lastRecs = getMetrics(new MetricsCollectorImpl(), true);
     }
 
-    synchronized(this) {
-      updateAttrCache();
-      if (getAllMetrics) {
-        updateInfoCache();
+    synchronized (this) {
+      if (lastRecs != null) {
+        updateAttrCache(lastRecs);
+        updateInfoCache(lastRecs);
       }
       jmxCacheTS = Time.now();
-      lastRecs = null;  // in case regular interval update is not running
       lastRecsCleared = true;
     }
   }
 
   Iterable<MetricsRecordImpl> getMetrics(MetricsCollectorImpl builder,
                                          boolean all) {
     builder.setRecordFilter(recordFilter).setMetricFilter(metricFilter);
-    synchronized(this) {
-      if (lastRecs == null && jmxCacheTS == 0) {
-        all = true; // Get all the metrics to populate the sink caches
-      }
-    }
     try {
       source.getMetrics(builder, all);
     } catch (Exception e) {
@@ -209,10 +205,7 @@ Iterable<MetricsRecordImpl> getMetrics(MetricsCollectorImpl builder,
         rb.add(t);
       }
     }
-    synchronized(this) {
-      lastRecs = builder.getRecords();
-      return lastRecs;
-    }
+    return builder.getRecords();
   }
 
   synchronized void stop() {
@@ -246,13 +239,15 @@ long getJmxCacheTTL() {
     return jmxCacheTTL;
   }
 
-  private void updateInfoCache() {
+  private void updateInfoCache(Iterable<MetricsRecordImpl> lastRecs) {
+    Preconditions.checkNotNull(lastRecs, "LastRecs should not be null");
     LOG.debug("Updating info cache...");
     infoCache = infoBuilder.reset(lastRecs).get();
     LOG.debug("Done");
   }
 
-  private int updateAttrCache() {
+  private int updateAttrCache(Iterable<MetricsRecordImpl> lastRecs) {
+    Preconditions.checkNotNull(lastRecs, "LastRecs should not be null");
     LOG.debug("Updating attr cache...");
     int recNo = 0;
     int numMetrics = 0;