HBASE-18656 First issues found by error-prone

hbase-common/src/main/java/org/apache/hadoop/hbase/util/ConcatenatedLists.java

@@ -18,9 +18,8 @@
  */
 package org.apache.hadoop.hbase.util;
 
-import java.lang.reflect.Array;
+import java.util.AbstractCollection;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.List;
 import java.util.NoSuchElementException;
 
@@ -33,7 +32,7 @@ import org.apache.hadoop.hbase.classification.InterfaceAudience;
  * NOTE: Doesn't implement list as it is not necessary for current usage, feel free to add.
  */
 @InterfaceAudience.Private
-public class ConcatenatedLists<T> implements Collection<T> {
+public class ConcatenatedLists<T> extends AbstractCollection<T> {
   protected final ArrayList<List<T>> components = new ArrayList<>();
   protected int size = 0;
 
@@ -55,77 +54,6 @@ public class ConcatenatedLists<T> implements Collection<T> {
     return this.size;
   }
 
-  @Override
-  public boolean isEmpty() {
-    return this.size == 0;
-  }
-
-  @Override
-  public boolean contains(Object o) {
-    for (List<T> component : this.components) {
-      if (component.contains(o)) return true;
-    }
-    return false;
-  }
-
-  @Override
-  public boolean containsAll(Collection<?> c) {
-    for (Object o : c) {
-      if (!contains(o)) return false;
-    }
-    return true;
-  }
-
-  @Override
-  public Object[] toArray() {
-    return toArray((Object[])Array.newInstance(Object.class, this.size));
-  }
-
-  @Override
-  @SuppressWarnings("unchecked")
-  public <U> U[] toArray(U[] a) {
-    U[] result = (a.length == this.size()) ? a
-        : (U[])Array.newInstance(a.getClass().getComponentType(), this.size);
-    int i = 0;
-    for (List<T> component : this.components) {
-      for (T t : component) {
-        result[i] = (U)t;
-        ++i;
-      }
-    }
-    return result;
-  }
-
-  @Override
-  public boolean add(T e) {
-    throw new UnsupportedOperationException();
-  }
-
-  @Override
-  public boolean remove(Object o) {
-    throw new UnsupportedOperationException();
-  }
-
-  @Override
-  public boolean addAll(Collection<? extends T> c) {
-    throw new UnsupportedOperationException();
-  }
-
-  @Override
-  public boolean removeAll(Collection<?> c) {
-    throw new UnsupportedOperationException();
-  }
-
-  @Override
-  public boolean retainAll(Collection<?> c) {
-    throw new UnsupportedOperationException();
-  }
-
-  @Override
-  public void clear() {
-    throw new UnsupportedOperationException();
-  }
-
   @Override
   public java.util.Iterator<T> iterator() {
     return new Iterator();

hbase-common/src/test/java/org/apache/hadoop/hbase/TestChoreService.java

@@ -24,6 +24,8 @@ import static org.junit.Assert.assertTrue;
 
 import java.util.concurrent.TimeUnit;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.hbase.TestChoreService.ScheduledChoreSamples.CountingChore;
 import org.apache.hadoop.hbase.TestChoreService.ScheduledChoreSamples.DoNothingChore;
 import org.apache.hadoop.hbase.TestChoreService.ScheduledChoreSamples.FailInitialChore;
@@ -36,6 +38,8 @@ import org.junit.experimental.categories.Category;
 
 @Category(SmallTests.class)
 public class TestChoreService {
+  public static final Log log = LogFactory.getLog(TestChoreService.class);
+
   /**
    * A few ScheduledChore samples that are useful for testing with ChoreService
    */
@@ -75,7 +79,7 @@ public class TestChoreService {
         try {
           Thread.sleep(getPeriod() * 2);
         } catch (InterruptedException e) {
-          e.printStackTrace();
+          log.warn("", e);
         }
         return true;
       }
@@ -85,7 +89,7 @@ public class TestChoreService {
         try {
           Thread.sleep(getPeriod() * 2);
         } catch (InterruptedException e) {
-          //e.printStackTrace();
+          log.warn("", e);
         }
       }
     }
@@ -126,7 +130,7 @@ public class TestChoreService {
         try {
           Thread.sleep(sleepTime);
         } catch (InterruptedException e) {
-          e.printStackTrace();
+          log.warn("", e);
         }
         return true;
       }
@@ -136,7 +140,7 @@ public class TestChoreService {
         try {
           Thread.sleep(sleepTime);
         } catch (Exception e) {
-          System.err.println(e.getStackTrace());
+          log.warn("", e);
         }
       }
     }
@@ -174,7 +178,7 @@ public class TestChoreService {
       }
 
       private void outputTickCount() {
-        System.out.println("Chore: " + getName() + ". Count of chore calls: " + countOfChoreCalls);
+        log.info("Chore: " + getName() + ". Count of chore calls: " + countOfChoreCalls);
       }
 
       public int getCountOfChoreCalls() {

hbase-common/src/test/java/org/apache/hadoop/hbase/util/TestConcatenatedLists.java

@@ -67,7 +67,7 @@ public class TestConcatenatedLists {
     } catch (UnsupportedOperationException ex) {
     }
     try {
-      c.retainAll(Arrays.asList(0L, 1L));
+      c.retainAll(Arrays.asList(0L, 2L));
       fail("Should throw");
     } catch (UnsupportedOperationException ex) {
     }
@@ -118,9 +118,11 @@ public class TestConcatenatedLists {
     verify(c, 7);
   }
 
+  @SuppressWarnings("ModifyingCollectionWithItself")
   private void verify(ConcatenatedLists<Long> c, int last) {
     assertEquals((last == -1), c.isEmpty());
     assertEquals(last + 1, c.size());
+    // This check is O(n^2), test with care
     assertTrue(c.containsAll(c));
     Long[] array = c.toArray(new Long[c.size()]);
     List<Long> all = new ArrayList<>();

hbase-common/src/test/java/org/apache/hadoop/hbase/util/TestDrainBarrier.java

@@ -46,7 +46,7 @@ public class TestDrainBarrier {
     DrainBarrier barrier = new DrainBarrier();
     try {
       barrier.endOp();
-      fail("Should have asserted");
+      throw new Error("Should have asserted");
     } catch (AssertionError e) {
     }
 
@@ -56,7 +56,7 @@ public class TestDrainBarrier {
     barrier.endOp();
     try {
       barrier.endOp();
-      fail("Should have asserted");
+      throw new Error("Should have asserted");
     } catch (AssertionError e) {
     }
   }
@@ -108,7 +108,7 @@ public class TestDrainBarrier {
     barrier.stopAndDrainOpsOnce();
     try {
       barrier.stopAndDrainOpsOnce();
-      fail("Should have asserted");
+      throw new Error("Should have asserted");
     } catch (AssertionError e) {
     }
   }