HBASE-12748 RegionCoprocessorHost.execOperation creates too many iterator objects

hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java

@@ -26,7 +26,6 @@ import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
-import java.util.SortedSet;
 import java.util.TreeSet;
 import java.util.UUID;
 import java.util.concurrent.ConcurrentSkipListSet;
@@ -49,7 +48,7 @@ import org.apache.hadoop.hbase.client.HTable;
 import org.apache.hadoop.hbase.client.HTableWrapper;
 import org.apache.hadoop.hbase.client.Table;
 import org.apache.hadoop.hbase.util.CoprocessorClassLoader;
-import org.apache.hadoop.hbase.util.SortedCopyOnWriteSet;
+import org.apache.hadoop.hbase.util.SortedList;
 import org.apache.hadoop.hbase.util.VersionInfo;
 
 /**
@@ -82,8 +81,8 @@ public abstract class CoprocessorHost<E extends CoprocessorEnvironment> {
   private static final Log LOG = LogFactory.getLog(CoprocessorHost.class);
   protected Abortable abortable;
   /** Ordered set of loaded coprocessors with lock */
-  protected SortedSet<E> coprocessors =
-      new SortedCopyOnWriteSet<E>(new EnvironmentPriorityComparator());
+  protected SortedList<E> coprocessors =
+      new SortedList<E>(new EnvironmentPriorityComparator());
   protected Configuration conf;
   // unique file prefix to use for local copies of jars when classloading
   protected String pathPrefix;

hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java

@@ -1079,7 +1079,9 @@ public class MasterCoprocessorHost
   private boolean execOperation(final CoprocessorOperation ctx) throws IOException {
     if (ctx == null) return false;
     boolean bypass = false;
-    for (MasterEnvironment env: coprocessors) {
+    List<MasterEnvironment> envs = coprocessors.get();
+    for (int i = 0; i < envs.size(); i++) {
+      MasterEnvironment env = envs.get(i);
       if (env.getInstance() instanceof MasterObserver) {
         ctx.prepare(env);
         Thread currentThread = Thread.currentThread();

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java

@@ -1664,7 +1664,9 @@ public class RegionCoprocessorHost
   private boolean execOperation(final boolean earlyExit, final CoprocessorOperation ctx)
       throws IOException {
     boolean bypass = false;
-    for (RegionEnvironment env: coprocessors) {
+    List<RegionEnvironment> envs = coprocessors.get();
+    for (int i = 0; i < envs.size(); i++) {
+      RegionEnvironment env = envs.get(i);
       Coprocessor observer = env.getInstance();
       if (ctx.hasCall(observer)) {
         ctx.prepare(env);

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerCoprocessorHost.java

@@ -243,9 +243,10 @@ public class RegionServerCoprocessorHost extends
 
   private boolean execOperation(final CoprocessorOperation ctx) throws IOException {
     if (ctx == null) return false;
-
     boolean bypass = false;
-    for (RegionServerEnvironment env: coprocessors) {
+    List<RegionServerEnvironment> envs = coprocessors.get();
+    for (int i = 0; i < envs.size(); i++) {
+      RegionServerEnvironment env = envs.get(i);
       if (env.getInstance() instanceof RegionServerObserver) {
         ctx.prepare(env);
         Thread currentThread = Thread.currentThread();

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCoprocessorHost.java

@@ -21,13 +21,13 @@
 package org.apache.hadoop.hbase.regionserver.wal;
 
 import java.io.IOException;
+import java.util.List;
 
 import org.apache.hadoop.hbase.Coprocessor;
 import org.apache.hadoop.hbase.HRegionInfo;
 import org.apache.hadoop.hbase.coprocessor.*;
 import org.apache.hadoop.hbase.classification.InterfaceAudience;
 import org.apache.hadoop.conf.Configuration;
-
 import org.apache.hadoop.hbase.wal.WAL;
 import org.apache.hadoop.hbase.wal.WALKey;
 
@@ -120,7 +120,9 @@ public class WALCoprocessorHost
     boolean bypass = false;
     if (this.coprocessors == null || this.coprocessors.isEmpty()) return bypass;
     ObserverContext<WALCoprocessorEnvironment> ctx = null;
-    for (WALEnvironment env: coprocessors) {
+    List<WALEnvironment> envs = coprocessors.get();
+    for (int i = 0; i < envs.size(); i++) {
+      WALEnvironment env = envs.get(i);
       if (env.getInstance() instanceof WALObserver) {
         final WALObserver observer = (WALObserver)env.getInstance();
         ctx = ObserverContext.createAndPrepare(env, ctx);
@@ -162,7 +164,9 @@ public class WALCoprocessorHost
       throws IOException {
     if (this.coprocessors == null || this.coprocessors.isEmpty()) return;
     ObserverContext<WALCoprocessorEnvironment> ctx = null;
-    for (WALEnvironment env: coprocessors) {
+    List<WALEnvironment> envs = coprocessors.get();
+    for (int i = 0; i < envs.size(); i++) {
+      WALEnvironment env = envs.get(i);
       if (env.getInstance() instanceof WALObserver) {
         final WALObserver observer = (WALObserver)env.getInstance();
         ctx = ObserverContext.createAndPrepare(env, ctx);

hbase-server/src/main/java/org/apache/hadoop/hbase/util/SortedList.java

@@ -0,0 +1,242 @@
+/*
+ * 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.hbase.util;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.RandomAccess;
+
+/**
+ * Simple sorted list implementation that uses {@link java.util.ArrayList} as
+ * the underlying collection so we can support RandomAccess. All mutations
+ * create a new copy of the <code>ArrayList</code> instance, so can be
+ * expensive. This class is only intended for use on small, very rarely
+ * written collections that expect highly concurrent reads.
+ * <p>
+ * Read operations are performed on a reference to the internal list at the
+ * time of invocation, so will not see any mutations to the collection during
+ * their operation. Iterating over list elements manually using the
+ * RandomAccess pattern involves multiple operations. For this to be safe get
+ * a reference to the internal list first using get(). 
+ * <p>
+ * If constructed with a {@link java.util.Comparator}, the list will be sorted
+ * using the comparator. Adding or changing an element using an index will
+ * trigger a resort.
+ * <p>
+ * Iterators are read-only. They cannot be used to remove elements.
+ */
+public class SortedList<E> implements List<E>, RandomAccess {
+  private volatile List<E> list;
+  private final Comparator<? super E> comparator;
+
+  /**
+   * Constructs an empty list with the default initial capacity that will be
+   * sorted using the given comparator.
+   *
+   * @param comparator the comparator
+   */
+  public SortedList(Comparator<? super E> comparator) {
+    this.list = Collections.emptyList();
+    this.comparator = comparator;
+  }
+
+  /**
+   * Constructs a list containing the elements of the given collection, in the
+   * order returned by the collection's iterator, that will be sorted with the
+   * given comparator.
+   *
+   * @param c the collection
+   * @param comparator the comparator
+   */
+  public SortedList(Collection<? extends E> c, Comparator<? super E> comparator) {
+    this.list = Collections.unmodifiableList(new ArrayList<E>(c));
+    this.comparator = comparator;
+  }
+
+  /**
+   * Returns a reference to the unmodifiable list currently backing the SortedList.
+   * Changes to the SortedList will not be reflected in this list. Use this
+   * method to get a reference for iterating over using the RandomAccess
+   * pattern.
+   */
+  public List<E> get() {
+    return list;
+  }
+
+  @Override
+  public int size() {
+    return list.size();
+  }
+
+  @Override
+  public boolean isEmpty() {
+    return list.isEmpty();
+  }
+
+  @Override
+  public boolean contains(Object o) {
+    return list.contains(o);
+  }
+
+  @Override
+  public Iterator<E> iterator() {
+    return list.iterator();
+  }
+
+  @Override
+  public Object[] toArray() {
+    return list.toArray();
+  }
+
+  @Override
+  public <T> T[] toArray(T[] a) {
+    return list.toArray(a);
+  }
+
+  @Override
+  public synchronized boolean add(E e) {
+    ArrayList<E> newList = new ArrayList<E>(list);
+    boolean changed = newList.add(e);
+    if (changed) {
+      Collections.sort(newList, comparator);
+    }
+    list = Collections.unmodifiableList(newList);
+    return changed;
+  }
+
+  @Override
+  public synchronized boolean remove(Object o) {
+    ArrayList<E> newList = new ArrayList<E>(list);
+    // Removals in ArrayList won't break sorting
+    boolean changed = newList.remove(o);
+    list = Collections.unmodifiableList(newList);
+    return changed;
+  }
+
+  @Override
+  public boolean containsAll(Collection<?> c) {
+    return list.containsAll(c);
+  }
+
+  @Override
+  public synchronized boolean addAll(Collection<? extends E> c) {
+    ArrayList<E> newList = new ArrayList<E>(list);
+    boolean changed = newList.addAll(c);
+    if (changed) {
+      Collections.sort(newList, comparator);
+    }
+    list = Collections.unmodifiableList(newList);
+    return changed;
+  }
+
+  @Override
+  public synchronized boolean addAll(int index, Collection<? extends E> c) {
+    ArrayList<E> newList = new ArrayList<E>(list);
+    boolean changed = newList.addAll(index, c);
+    if (changed) {
+      Collections.sort(newList, comparator);
+    }
+    list = Collections.unmodifiableList(newList);
+    return changed;
+  }
+
+  @Override
+  public synchronized boolean removeAll(Collection<?> c) {
+    ArrayList<E> newList = new ArrayList<E>(list);
+    // Removals in ArrayList won't break sorting
+    boolean changed = newList.removeAll(c);
+    list = Collections.unmodifiableList(newList);
+    return changed;
+  }
+
+  @Override
+  public synchronized boolean retainAll(Collection<?> c) {
+    ArrayList<E> newList = new ArrayList<E>(list);
+    // Removals in ArrayList won't break sorting
+    boolean changed = newList.retainAll(c);
+    list = Collections.unmodifiableList(newList);
+    return changed;
+  }
+
+  @Override
+  public synchronized void clear() {
+    list = Collections.emptyList();
+  }
+
+  @Override
+  public E get(int index) {
+    return list.get(index);
+  }
+
+  @Override
+  public synchronized E set(int index, E element) {
+    ArrayList<E> newList = new ArrayList<E>(list);
+    E result = newList.set(index, element);
+    Collections.sort(list, comparator);
+    list = Collections.unmodifiableList(newList);
+    return result;
+  }
+
+  @Override
+  public synchronized void add(int index, E element) {
+    ArrayList<E> newList = new ArrayList<E>(list);
+    newList.add(index, element);
+    Collections.sort(list, comparator);
+    list = Collections.unmodifiableList(newList);
+  }
+
+  @Override
+  public synchronized E remove(int index) {
+    ArrayList<E> newList = new ArrayList<E>(list);
+    // Removals in ArrayList won't break sorting
+    E result = newList.remove(index);
+    list = Collections.unmodifiableList(newList);
+    return result;
+  }
+
+  @Override
+  public int indexOf(Object o) {
+    return list.indexOf(o);
+  }
+
+  @Override
+  public int lastIndexOf(Object o) {
+    return list.lastIndexOf(o);
+  }
+
+  @Override
+  public ListIterator<E> listIterator() {
+    return list.listIterator();
+  }
+
+  @Override
+  public ListIterator<E> listIterator(int index) {
+    return list.listIterator(index);
+  }
+
+  @Override
+  public List<E> subList(int fromIndex, int toIndex) {
+    return list.subList(fromIndex, toIndex);
+  }
+}

hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestSortedList.java

@@ -0,0 +1,173 @@
+/*
+ *
+ * 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.hbase.util;
+
+import static org.junit.Assert.*;
+
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.List;
+import java.util.ListIterator;
+
+import com.google.common.collect.Lists;
+
+import org.apache.hadoop.hbase.testclassification.MiscTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category({MiscTests.class, SmallTests.class})
+public class TestSortedList {
+
+  static class StringComparator implements Comparator<String> {
+    @Override
+    public int compare(String o1, String o2) {
+      return o1.compareTo(o2);
+    }
+  }
+
+  @Test
+  public void testSorting() throws Exception {
+    SortedList<String> list = new SortedList<String>(new StringComparator());
+    list.add("c");
+    list.add("d");
+    list.add("a");
+    list.add("b");
+
+    assertEquals(4, list.size());
+    assertArrayEquals(new String[]{"a", "b", "c", "d"}, list.toArray(new String[4]));
+
+    list.add("c");
+    assertEquals(5, list.size());
+    assertArrayEquals(new String[]{"a", "b", "c", "c", "d"}, list.toArray(new String[5]));
+
+    // Test that removal from head or middle maintains sort
+    list.remove("b");
+    assertEquals(4, list.size());
+    assertArrayEquals(new String[]{"a", "c", "c", "d"}, list.toArray(new String[4]));
+    list.remove("c");
+    assertEquals(3, list.size());
+    assertArrayEquals(new String[]{"a", "c", "d"}, list.toArray(new String[3]));
+    list.remove("a");
+    assertEquals(2, list.size());
+    assertArrayEquals(new String[]{"c", "d"}, list.toArray(new String[2]));
+  }
+
+  @Test
+  public void testReadOnlyIterators() throws Exception {
+    SortedList<String> list = new SortedList<String>(
+        Lists.newArrayList("a", "b", "c", "d", "e"), new StringComparator());
+
+    Iterator<String> i = list.iterator();
+    i.next();
+    try {
+      i.remove();
+      fail("Iterator should have thrown an exception");
+    } catch (UnsupportedOperationException e) {
+      // ok
+    }
+
+    ListIterator<String> li = list.listIterator();
+    li.next();
+    try {
+      li.add("a");
+      fail("Iterator should have thrown an exception");
+    } catch (UnsupportedOperationException e) {
+      // ok
+    }
+    try {
+      li.set("b");
+      fail("Iterator should have thrown an exception");
+    } catch (UnsupportedOperationException e) {
+      // ok
+    }
+    try {
+      li.remove();
+      fail("Iterator should have thrown an exception");
+    } catch (UnsupportedOperationException e) {
+      // ok
+    }
+  }
+
+  @Test
+  public void testIteratorIsolation() throws Exception {
+    SortedList<String> list = new SortedList<String>(
+        Lists.newArrayList("a", "b", "c", "d", "e"), new StringComparator());
+
+    // isolation of remove()
+    Iterator<String> iter = list.iterator();
+    list.remove("c");
+    boolean found = false;
+    while (iter.hasNext() && !found) {
+      found = "c".equals(iter.next());
+    }
+    assertTrue(found);
+
+    iter = list.iterator();
+    found = false;
+    while (iter.hasNext() && !found) {
+      found = "c".equals(iter.next());
+    }
+    assertFalse(found);
+
+    // isolation of add()
+    iter = list.iterator();
+    list.add("f");
+    found = false;
+    while (iter.hasNext() && !found) {
+      String next = iter.next();
+      found = "f".equals(next);
+    }
+    assertFalse(found);
+
+    // isolation of addAll()
+    iter = list.iterator();
+    list.addAll(Lists.newArrayList("g", "h", "i"));
+    found = false;
+    while (iter.hasNext() && !found) {
+      String next = iter.next();
+      found = "g".equals(next) || "h".equals(next) || "i".equals(next);
+    }
+    assertFalse(found);
+
+    // isolation of clear()
+    iter = list.iterator();
+    list.clear();
+    assertEquals(0, list.size());
+    int size = 0;
+    while (iter.hasNext()) {
+      iter.next();
+      size++;
+    }
+    assertTrue(size > 0);
+  }
+
+  @Test
+  public void testRandomAccessIsolation() throws Exception {
+    SortedList<String> list = new SortedList<String>(
+        Lists.newArrayList("a", "b", "c"), new StringComparator());
+    List<String> innerList = list.get();
+    assertEquals("a", innerList.get(0));
+    assertEquals("b", innerList.get(1));
+    list.clear();
+    assertEquals("c", innerList.get(2));
+  }
+}
+