MAPREDUCE-3749. ConcurrentModificationException in counter groups.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1238734 13f79535-47bb-0310-9956-ffa450edef68

hadoop-mapreduce-project/CHANGES.txt

@@ -625,7 +625,10 @@ Release 0.23.1 - Unreleased
     (ramya via acmurthy) 
 
     MAPREDUCE-3764. Fixed resource usage metrics for queues and users.
-    (acmurthy) 
+    (acmurthy)
+
+    MAPREDUCE-3749. ConcurrentModificationException in counter groups.
+    (tomwhite)
 
 Release 0.23.0 - 2011-11-01 
 

hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/counters/AbstractCounterGroup.java

@@ -24,6 +24,7 @@ import java.io.IOException;
 import java.util.Iterator;
 import java.util.Map;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterators;
 import com.google.common.collect.Maps;
 
@@ -56,7 +57,7 @@ public abstract class AbstractCounterGroup<T extends Counter>
   }
 
   @Override
-  public synchronized String getName() {
+  public String getName() {
     return name;
   }
 
@@ -95,7 +96,7 @@ public abstract class AbstractCounterGroup<T extends Counter>
   }
 
   @Override
-  public T findCounter(String counterName, String displayName) {
+  public synchronized T findCounter(String counterName, String displayName) {
     String saveName = limits.filterCounterName(counterName);
     T counter = findCounterImpl(saveName, false);
     if (counter == null) {
@@ -109,7 +110,7 @@ public abstract class AbstractCounterGroup<T extends Counter>
     return findCounterImpl(limits.filterCounterName(counterName), create);
   }
 
-  private T findCounterImpl(String counterName, boolean create) {
+  private synchronized T findCounterImpl(String counterName, boolean create) {
     T counter = counters.get(counterName);
     if (counter == null && create) {
       String localized =
@@ -142,7 +143,7 @@ public abstract class AbstractCounterGroup<T extends Counter>
 
   @Override
   public synchronized Iterator<T> iterator() {
-    return counters.values().iterator();
+    return ImmutableSet.copyOf(counters.values()).iterator();
   }
 
   /**

hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/counters/AbstractCounters.java

@@ -24,6 +24,7 @@ import java.io.IOException;
 import java.util.Iterator;
 import java.util.Map;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Iterators;
 import com.google.common.collect.Maps;
@@ -179,13 +180,14 @@ public abstract class AbstractCounters<C extends Counter,
    * @return Set of counter names.
    */
   public synchronized Iterable<String> getGroupNames() {
-    return Iterables.concat(fgroups.keySet(), groups.keySet());
+    return Iterables.concat(ImmutableSet.copyOf(fgroups.keySet()),
+                            ImmutableSet.copyOf(groups.keySet()));
   }
 
   @Override
-  public Iterator<G> iterator() {
-    return Iterators.concat(fgroups.values().iterator(),
-                            groups.values().iterator());
+  public synchronized Iterator<G> iterator() {
+    return Iterators.concat(ImmutableSet.copyOf(fgroups.values()).iterator(),
+                            ImmutableSet.copyOf(groups.values()).iterator());
   }
 
   /**

hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapred/TestCounters.java

@@ -21,9 +21,11 @@ import static org.junit.Assert.assertEquals;
 
 import java.io.IOException;
 import java.text.ParseException;
+import java.util.Iterator;
 import java.util.Random;
 
 import org.apache.hadoop.mapred.Counters.Counter;
+import org.apache.hadoop.mapred.Counters.Group;
 import org.apache.hadoop.mapreduce.FileSystemCounter;
 import org.apache.hadoop.mapreduce.JobCounter;
 import org.apache.hadoop.mapreduce.TaskCounter;
@@ -71,8 +73,6 @@ public class TestCounters {
     // Check for recovery from string
     assertEquals("Recovered counter does not match on content", 
                  counter, recoveredCounter);
-    assertEquals("recovered counter has wrong hash code",
-                 counter.hashCode(), recoveredCounter.hashCode());
   }
   
   @Test
@@ -159,6 +159,28 @@ public class TestCounters {
         "FILE_BYTES_READ").getValue());
   }
   
+  @SuppressWarnings("deprecation")
+  @Test
+  public void testCounterIteratorConcurrency() {
+    Counters counters = new Counters();
+    counters.incrCounter("group1", "counter1", 1);
+    Iterator<Group> iterator = counters.iterator();
+    counters.incrCounter("group2", "counter2", 1);
+    iterator.next();
+  }
+  
+  
+  @SuppressWarnings("deprecation")
+  @Test
+  public void testGroupIteratorConcurrency() {
+    Counters counters = new Counters();
+    counters.incrCounter("group1", "counter1", 1);
+    Group group = counters.getGroup("group1");
+    Iterator<Counter> iterator = group.iterator();
+    counters.incrCounter("group1", "counter2", 1);
+    iterator.next();
+  }
+  
   public static void main(String[] args) throws IOException {
     new TestCounters().testCounters();
   }