YARN-4204. ConcurrentModificationException in FairSchedulerQueueInfo. (adhoot)

hadoop-yarn-project/CHANGES.txt

@@ -902,6 +902,8 @@ Release 2.8.0 - UNRELEASED
     YARN-4044. Running applications information changes such as movequeue is not published to 
     TimeLine server. (Sunil G via rohithsharmaks)
 
+    YARN-4204. ConcurrentModificationException in FairSchedulerQueueInfo. (adhoot)
+
 Release 2.7.2 - UNRELEASED
 
   INCOMPATIBLE CHANGES

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSLeafQueue.java

@@ -70,6 +70,7 @@ public class FSLeafQueue extends FSQueue {
   private Resource amResourceUsage;
 
   private final ActiveUsersManager activeUsersManager;
+  public static final List<FSQueue> EMPTY_LIST = Collections.emptyList();
 
   public FSLeafQueue(String name, FairScheduler scheduler,
       FSParentQueue parent) {
@@ -383,7 +384,7 @@ public RMContainer preemptContainer() {
 
   @Override
   public List<FSQueue> getChildQueues() {
-    return new ArrayList<FSQueue>(1);
+    return EMPTY_LIST;
   }
   
   @Override

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSParentQueue.java

@@ -27,6 +27,7 @@
 import java.util.concurrent.locks.ReadWriteLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 
+import com.google.common.collect.ImmutableList;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.classification.InterfaceAudience.Private;
@@ -279,7 +280,7 @@ public RMContainer preemptContainer() {
   public List<FSQueue> getChildQueues() {
     readLock.lock();
     try {
-      return Collections.unmodifiableList(childQueues);
+      return ImmutableList.copyOf(childQueues);
     } finally {
       readLock.unlock();
     }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueueManager.java

@@ -28,6 +28,7 @@
 
 import javax.xml.parsers.ParserConfigurationException;
 
+import com.google.common.collect.ImmutableList;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.classification.InterfaceAudience.Private;
@@ -295,17 +296,18 @@ private boolean removeQueueIfEmpty(FSQueue queue) {
    * Remove a queue and all its descendents.
    */
   private void removeQueue(FSQueue queue) {
-    if (queue instanceof FSLeafQueue) {
-      leafQueues.remove(queue);
-    } else {
-      List<FSQueue> childQueues = queue.getChildQueues();
-      while (!childQueues.isEmpty()) {
-        removeQueue(childQueues.get(0));
+    synchronized (queues) {
+      if (queue instanceof FSLeafQueue) {
+        leafQueues.remove(queue);
+      } else {
+        for (FSQueue childQueue:queue.getChildQueues()) {
+          removeQueue(childQueue);
+        }
       }
+      queues.remove(queue.getName());
+      FSParentQueue parent = queue.getParent();
+      parent.removeChildQueue(queue);
     }
-    queues.remove(queue.getName());
-    FSParentQueue parent = queue.getParent();
-    parent.removeChildQueue(queue);
   }
   
   /**
@@ -360,7 +362,9 @@ public Collection<FSLeafQueue> getLeafQueues() {
    * Get a collection of all queues
    */
   public Collection<FSQueue> getQueues() {
-    return queues.values();
+    synchronized (queues) {
+      return ImmutableList.copyOf(queues.values());
+    }
   }
   
   private String ensureRootPrefix(String name) {

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/TestFSParentQueue.java

@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair;
+
+import org.apache.hadoop.yarn.util.SystemClock;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class TestFSParentQueue {
+
+  private FairSchedulerConfiguration conf;
+  private QueueManager queueManager;
+  private Set<FSQueue> notEmptyQueues;
+
+  @Before
+  public void setUp() throws Exception {
+    conf = new FairSchedulerConfiguration();
+    FairScheduler scheduler = mock(FairScheduler.class);
+    AllocationConfiguration allocConf = new AllocationConfiguration(conf);
+    when(scheduler.getAllocationConfiguration()).thenReturn(allocConf);
+    when(scheduler.getConf()).thenReturn(conf);
+    SystemClock clock = new SystemClock();
+    when(scheduler.getClock()).thenReturn(clock);
+    notEmptyQueues = new HashSet<FSQueue>();
+    queueManager = new QueueManager(scheduler) {
+      @Override
+      public boolean isEmpty(FSQueue queue) {
+        return !notEmptyQueues.contains(queue);
+      }
+    };
+    FSQueueMetrics.forQueue("root", null, true, conf);
+    queueManager.initialize(conf);
+  }
+
+  @Test
+  public void testConcurrentChangeToGetChildQueue() {
+
+    queueManager.getLeafQueue("parent.child", true);
+    queueManager.getLeafQueue("parent.child2", true);
+    FSParentQueue test = queueManager.getParentQueue("parent", false);
+    assertEquals(2, test.getChildQueues().size());
+
+    boolean first = true;
+    int childQueuesFound = 0;
+    for (FSQueue childQueue:test.getChildQueues()) {
+      if (first) {
+        first = false;
+        queueManager.getLeafQueue("parent.child3", true);
+      }
+      childQueuesFound++;
+    }
+
+    assertEquals(2, childQueuesFound);
+    assertEquals(3, test.getChildQueues().size());
+  }
+}