You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by ap...@apache.org on 2015/09/25 23:00:10 UTC
hbase git commit: HBASE-12748 RegionCoprocessorHost.execOperation
creates too many iterator objects
Repository: hbase
Updated Branches:
refs/heads/0.98 40bfeec30 -> c38c47c00
HBASE-12748 RegionCoprocessorHost.execOperation creates too many iterator objects
Project: http://git-wip-us.apache.org/repos/asf/hbase/repo
Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/c38c47c0
Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/c38c47c0
Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/c38c47c0
Branch: refs/heads/0.98
Commit: c38c47c0007d3f1ff40f38558834fe6fa3ad589d
Parents: 40bfeec
Author: Andrew Purtell <ap...@apache.org>
Authored: Tue Sep 22 22:21:38 2015 -0700
Committer: Andrew Purtell <ap...@apache.org>
Committed: Fri Sep 25 13:56:43 2015 -0700
----------------------------------------------------------------------
.../hbase/coprocessor/CoprocessorHost.java | 7 +-
.../hbase/master/MasterCoprocessorHost.java | 4 +-
.../regionserver/RegionCoprocessorHost.java | 4 +-
.../RegionServerCoprocessorHost.java | 5 +-
.../regionserver/wal/WALCoprocessorHost.java | 9 +-
.../apache/hadoop/hbase/util/SortedList.java | 242 +++++++++++++++++++
.../hadoop/hbase/util/TestSortedList.java | 172 +++++++++++++
7 files changed, 433 insertions(+), 10 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/hbase/blob/c38c47c0/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
index 0d51dd1..d820874 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
@@ -27,7 +27,6 @@ import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
-import java.util.SortedSet;
import java.util.TreeSet;
import java.util.UUID;
import java.util.concurrent.ExecutorService;
@@ -67,7 +66,7 @@ import org.apache.hadoop.hbase.filter.CompareFilter.CompareOp;
import org.apache.hadoop.hbase.ipc.CoprocessorRpcChannel;
import org.apache.hadoop.hbase.util.Bytes;
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;
import org.apache.hadoop.io.MultipleIOException;
@@ -106,8 +105,8 @@ public abstract class CoprocessorHost<E extends CoprocessorEnvironment> {
protected 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;
http://git-wip-us.apache.org/repos/asf/hbase/blob/c38c47c0/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java
index a9bb081..b431c7c 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java
@@ -880,7 +880,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();
http://git-wip-us.apache.org/repos/asf/hbase/blob/c38c47c0/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
index e7228f3..829b611 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
@@ -1684,7 +1684,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)) {
long startTime = System.nanoTime();
http://git-wip-us.apache.org/repos/asf/hbase/blob/c38c47c0/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerCoprocessorHost.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerCoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerCoprocessorHost.java
index 99e5ed9..dfe9818 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerCoprocessorHost.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerCoprocessorHost.java
@@ -263,9 +263,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();
http://git-wip-us.apache.org/repos/asf/hbase/blob/c38c47c0/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCoprocessorHost.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCoprocessorHost.java
index 07f4a0d..379704f 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCoprocessorHost.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCoprocessorHost.java
@@ -21,6 +21,7 @@
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;
@@ -104,7 +105,9 @@ public class WALCoprocessorHost
throws IOException {
boolean bypass = false;
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
org.apache.hadoop.hbase.coprocessor.WALObserver) {
ctx = ObserverContext.createAndPrepare(env, ctx);
@@ -137,7 +140,9 @@ public class WALCoprocessorHost
public void postWALWrite(final HRegionInfo info, final HLogKey logKey, final WALEdit logEdit)
throws IOException {
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
org.apache.hadoop.hbase.coprocessor.WALObserver) {
ctx = ObserverContext.createAndPrepare(env, ctx);
http://git-wip-us.apache.org/repos/asf/hbase/blob/c38c47c0/hbase-server/src/main/java/org/apache/hadoop/hbase/util/SortedList.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/SortedList.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/SortedList.java
new file mode 100644
index 0000000..75d77ed
--- /dev/null
+++ b/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);
+ }
+}
http://git-wip-us.apache.org/repos/asf/hbase/blob/c38c47c0/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestSortedList.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestSortedList.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestSortedList.java
new file mode 100644
index 0000000..168d879
--- /dev/null
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestSortedList.java
@@ -0,0 +1,172 @@
+/*
+ *
+ * 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.SmallTests;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(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));
+ }
+}
+