HADOOP-11361. Fix a race condition in MetricsSourceAdapter.updateJmxCache. Contributed by Vinayakumar B, Yongjun Zhang, and Brahma Reddy Battula. (ozawa)

(cherry picked from commit 77ffe7621212be9f462ca37a542a13d167eca4e0)
(cherry picked from commit 7c5e7433e143a27763901e471e9f39f41bb24323)

hadoop-common-project/hadoop-common/CHANGES.txt

@@ -62,6 +62,9 @@ Release 2.6.5 - UNRELEASED
 
     HADOOP-12482. Race condition in JMX cache update. (Tony Wu via lei)
 
+    HADOOP-11361. Fix a race condition in MetricsSourceAdapter.updateJmxCache.
+    (Vinayakumar B, Yongjun Zhang, and Brahma Reddy Battula via ozawa)
+
 Release 2.6.4 - 2016-02-11
 
   INCOMPATIBLE CHANGES

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSourceAdapter.java

@@ -31,6 +31,7 @@ import javax.management.ReflectionException;
 
 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,18 +175,19 @@ class MetricsSourceAdapter implements DynamicMBean {
       }
     }
 
+    // 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;
     }
   }
@@ -194,11 +195,6 @@ class MetricsSourceAdapter implements DynamicMBean {
   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);
     }
@@ -210,10 +206,7 @@ class MetricsSourceAdapter implements DynamicMBean {
         rb.add(t);
       }
     }
-    synchronized(this) {
-      lastRecs = builder.getRecords();
-      return lastRecs;
-    }
+    return builder.getRecords();
   }
 
   synchronized void stop() {
@@ -247,13 +240,15 @@ class MetricsSourceAdapter implements DynamicMBean {
     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;