hbase-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@apache.org
Subject hbase git commit: HBASE-15087 Fix hbase-common findbugs complaints
Date Tue, 12 Jan 2016 08:30:47 GMT
Repository: hbase
Updated Branches:
  refs/heads/master 8ee9158b5 -> 83c506d9d


HBASE-15087 Fix hbase-common findbugs complaints


Project: http://git-wip-us.apache.org/repos/asf/hbase/repo
Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/83c506d9
Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/83c506d9
Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/83c506d9

Branch: refs/heads/master
Commit: 83c506d9d49e900f3d4d5feca2ded135fb19b827
Parents: 8ee9158
Author: stack <stack@apache.org>
Authored: Tue Jan 12 00:30:29 2016 -0800
Committer: stack <stack@apache.org>
Committed: Tue Jan 12 00:30:42 2016 -0800

----------------------------------------------------------------------
 .../org/apache/hadoop/hbase/CellComparator.java | 33 ++++++++++++++------
 .../java/org/apache/hadoop/hbase/CellUtil.java  |  2 ++
 .../JitterScheduledThreadPoolExecutorImpl.java  | 14 ++++++++-
 .../apache/hadoop/hbase/OffheapKeyValue.java    |  4 +--
 .../org/apache/hadoop/hbase/ProcedureInfo.java  |  4 ++-
 .../hadoop/hbase/types/CopyOnWriteArrayMap.java | 10 +++++-
 .../hadoop/hbase/util/ByteBufferUtils.java      | 32 +++++++++++++++++--
 .../java/org/apache/hadoop/hbase/util/DNS.java  |  4 ++-
 .../hadoop/hbase/util/DynamicClassLoader.java   |  4 ++-
 .../apache/hadoop/hbase/util/UnsafeAccess.java  |  5 +--
 .../hadoop/hbase/util/TestByteBufferUtils.java  |  9 ++++++
 11 files changed, 100 insertions(+), 21 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/hbase/blob/83c506d9/hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparator.java
----------------------------------------------------------------------
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparator.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparator.java
index b179963..d869b3e 100644
--- a/hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparator.java
+++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/CellComparator.java
@@ -171,9 +171,14 @@ public class CellComparator implements Comparator<Cell>, Serializable
{
           right.getFamilyArray(), right.getFamilyOffset(), right.getFamilyLength());
     }
     if (right instanceof ByteBufferedCell) {
-      return -(ByteBufferUtils.compareTo(((ByteBufferedCell) right).getFamilyByteBuffer(),
-          ((ByteBufferedCell) right).getFamilyPosition(), right.getFamilyLength(),
-          left.getFamilyArray(), left.getFamilyOffset(), left.getFamilyLength()));
+      // Notice how we flip the order of the compare here. We used to negate the return value
but
+      // see what FindBugs says
+      // http://findbugs.sourceforge.net/bugDescriptions.html#RV_NEGATING_RESULT_OF_COMPARETO
+      // It suggest flipping the order to get same effect and 'safer'.
+      return ByteBufferUtils.compareTo(
+          left.getFamilyArray(), left.getFamilyOffset(), left.getFamilyLength(),
+          ((ByteBufferedCell)right).getFamilyByteBuffer(),
+          ((ByteBufferedCell)right).getFamilyPosition(), right.getFamilyLength());
     }
     return Bytes.compareTo(left.getFamilyArray(), left.getFamilyOffset(), left.getFamilyLength(),
         right.getFamilyArray(), right.getFamilyOffset(), right.getFamilyLength());
@@ -210,10 +215,14 @@ public class CellComparator implements Comparator<Cell>, Serializable
{
           right.getQualifierArray(), right.getQualifierOffset(), right.getQualifierLength());
     }
     if (right instanceof ByteBufferedCell) {
-      return -(ByteBufferUtils.compareTo(((ByteBufferedCell) right).getQualifierByteBuffer(),
-          ((ByteBufferedCell) right).getQualifierPosition(),
-          right.getQualifierLength(), left.getQualifierArray(), left.getQualifierOffset(),
-          left.getQualifierLength()));
+      // Notice how we flip the order of the compare here. We used to negate the return value
but
+      // see what FindBugs says
+      // http://findbugs.sourceforge.net/bugDescriptions.html#RV_NEGATING_RESULT_OF_COMPARETO
+      // It suggest flipping the order to get same effect and 'safer'.
+      return ByteBufferUtils.compareTo(left.getQualifierArray(),
+          left.getQualifierOffset(), left.getQualifierLength(),
+          ((ByteBufferedCell)right).getQualifierByteBuffer(),
+          ((ByteBufferedCell)right).getQualifierPosition(), right.getQualifierLength());
     }
     return Bytes.compareTo(left.getQualifierArray(), left.getQualifierOffset(),
         left.getQualifierLength(), right.getQualifierArray(), right.getQualifierOffset(),
@@ -331,9 +340,13 @@ public class CellComparator implements Comparator<Cell>, Serializable
{
           right.getRowArray(), right.getRowOffset(), right.getRowLength());
     }
     if (right instanceof ByteBufferedCell) {
-      return -(ByteBufferUtils.compareTo(((ByteBufferedCell) right).getRowByteBuffer(),
-          ((ByteBufferedCell) right).getRowPosition(), right.getRowLength(),
-          left.getRowArray(), left.getRowOffset(), left.getRowLength()));
+      // Notice how we flip the order of the compare here. We used to negate the return value
but
+      // see what FindBugs says
+      // http://findbugs.sourceforge.net/bugDescriptions.html#RV_NEGATING_RESULT_OF_COMPARETO
+      // It suggest flipping the order to get same effect and 'safer'.
+      return ByteBufferUtils.compareTo(left.getRowArray(), left.getRowOffset(), left.getRowLength(),
+          ((ByteBufferedCell)right).getRowByteBuffer(),
+          ((ByteBufferedCell)right).getRowPosition(), right.getRowLength());
     }
     return Bytes.compareTo(left.getRowArray(), left.getRowOffset(), left.getRowLength(),
         right.getRowArray(), right.getRowOffset(), right.getRowLength());

http://git-wip-us.apache.org/repos/asf/hbase/blob/83c506d9/hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
----------------------------------------------------------------------
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
index 1ec6afa..1b38b56 100644
--- a/hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
+++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/CellUtil.java
@@ -819,6 +819,8 @@ public final class CellUtil {
     }
 
     @Override
+    @edu.umd.cs.findbugs.annotations.SuppressWarnings(value="IT_NO_SUCH_ELEMENT",
+      justification="Intentional")
     public Tag next() {
       return null;
     }

http://git-wip-us.apache.org/repos/asf/hbase/blob/83c506d9/hbase-common/src/main/java/org/apache/hadoop/hbase/JitterScheduledThreadPoolExecutorImpl.java
----------------------------------------------------------------------
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/JitterScheduledThreadPoolExecutorImpl.java
b/hbase-common/src/main/java/org/apache/hadoop/hbase/JitterScheduledThreadPoolExecutorImpl.java
index 7e7239e..c330fa7 100644
--- a/hbase-common/src/main/java/org/apache/hadoop/hbase/JitterScheduledThreadPoolExecutorImpl.java
+++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/JitterScheduledThreadPoolExecutorImpl.java
@@ -93,6 +93,19 @@ public class JitterScheduledThreadPoolExecutorImpl extends ScheduledThreadPoolEx
     }
 
     @Override
+    public boolean equals(Object obj) {
+      if (obj == this) {
+        return true;
+      }
+      return obj instanceof Delayed? compareTo((Delayed)obj) == 0: false;
+    }
+
+    @Override
+    public int hashCode() {
+      return this.wrapped.hashCode();
+    }
+
+    @Override
     public void run() {
       wrapped.run();
     }
@@ -123,5 +136,4 @@ public class JitterScheduledThreadPoolExecutorImpl extends ScheduledThreadPoolEx
       return wrapped.get(timeout, unit);
     }
   }
-
 }

http://git-wip-us.apache.org/repos/asf/hbase/blob/83c506d9/hbase-common/src/main/java/org/apache/hadoop/hbase/OffheapKeyValue.java
----------------------------------------------------------------------
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/OffheapKeyValue.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/OffheapKeyValue.java
index ced1595..d060b02 100644
--- a/hbase-common/src/main/java/org/apache/hadoop/hbase/OffheapKeyValue.java
+++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/OffheapKeyValue.java
@@ -32,8 +32,8 @@ import org.apache.hadoop.hbase.util.ClassSize;
  * memory.
  */
 @InterfaceAudience.Private
-public class OffheapKeyValue extends ByteBufferedCell implements HeapSize, Cloneable,
-    SettableSequenceId, Streamable {
+public class OffheapKeyValue extends ByteBufferedCell
+  implements HeapSize, SettableSequenceId, Streamable {
 
   protected final ByteBuffer buf;
   protected final int offset;

http://git-wip-us.apache.org/repos/asf/hbase/blob/83c506d9/hbase-common/src/main/java/org/apache/hadoop/hbase/ProcedureInfo.java
----------------------------------------------------------------------
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/ProcedureInfo.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/ProcedureInfo.java
index 845a536..b7ea47e 100644
--- a/hbase-common/src/main/java/org/apache/hadoop/hbase/ProcedureInfo.java
+++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/ProcedureInfo.java
@@ -35,7 +35,7 @@ import org.apache.hadoop.hbase.util.NonceKey;
  */
 @InterfaceAudience.Public
 @InterfaceStability.Evolving
-public class ProcedureInfo {
+public class ProcedureInfo implements Cloneable {
   private final long procId;
   private final String procName;
   private final String procOwner;
@@ -72,6 +72,8 @@ public class ProcedureInfo {
     this.result = result;
   }
 
+  @edu.umd.cs.findbugs.annotations.SuppressWarnings(value="CN_IDIOM_NO_SUPER_CALL",
+      justification="Intentional; calling super class clone doesn't make sense here.")
   public ProcedureInfo clone() {
     return new ProcedureInfo(
       procId, procName, procOwner, procState, parentId, exception, lastUpdate, startTime,
result);

http://git-wip-us.apache.org/repos/asf/hbase/blob/83c506d9/hbase-common/src/main/java/org/apache/hadoop/hbase/types/CopyOnWriteArrayMap.java
----------------------------------------------------------------------
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/types/CopyOnWriteArrayMap.java
b/hbase-common/src/main/java/org/apache/hadoop/hbase/types/CopyOnWriteArrayMap.java
index 41056b2..8de39ae 100644
--- a/hbase-common/src/main/java/org/apache/hadoop/hbase/types/CopyOnWriteArrayMap.java
+++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/types/CopyOnWriteArrayMap.java
@@ -27,6 +27,7 @@ import java.util.Comparator;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.NavigableSet;
+import java.util.NoSuchElementException;
 import java.util.Set;
 import java.util.SortedSet;
 import java.util.concurrent.ConcurrentNavigableMap;
@@ -693,8 +694,10 @@ public class CopyOnWriteArrayMap<K, V> extends AbstractMap<K,
V>
     }
 
     @Override
+    @edu.umd.cs.findbugs.annotations.SuppressWarnings(value="EQ_ALWAYS_FALSE",
+      justification="Intentional")
     public boolean equals(Object o) {
-      return false;
+      return false; // FindBugs: Causes EQ_ALWAYS_FALSE. Suppressed.
     }
 
     @Override
@@ -771,7 +774,12 @@ public class CopyOnWriteArrayMap<K, V> extends AbstractMap<K,
V>
     }
 
     @Override
+    @edu.umd.cs.findbugs.annotations.SuppressWarnings(value="IT_NO_SUCH_ELEMENT",
+      justification="Intentional")
     public Entry<K, V> next() {
+      if (!hasNext()) {
+        throw new NoSuchElementException();
+      }
       return holder.entries[index++];
     }
 

http://git-wip-us.apache.org/repos/asf/hbase/blob/83c506d9/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java
----------------------------------------------------------------------
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java
b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java
index 62173c2..99e798a 100644
--- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java
+++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java
@@ -38,6 +38,7 @@ import sun.nio.ch.DirectBuffer;
  * Utility functions for working with byte buffers, such as reading/writing
  * variable-length long numbers.
  */
+@SuppressWarnings("restriction")
 @InterfaceAudience.Public
 @InterfaceStability.Evolving
 public final class ByteBufferUtils {
@@ -616,6 +617,33 @@ public final class ByteBufferUtils {
     return compareTo(buf1, o1, l1, buf2, o2, l2) == 0;
   }
 
+  public static int compareTo(byte [] buf1, int o1, int l1, ByteBuffer buf2, int o2, int
l2) {
+    // This method is nearly same as the compareTo that follows but hard sharing code given
+    // byte array and bytebuffer types and this is a hot code path
+    if (UNSAFE_UNALIGNED) {
+      long offset2Adj;
+      Object refObj2 = null;
+      if (buf2.isDirect()) {
+        offset2Adj = o2 + ((DirectBuffer)buf2).address();
+      } else {
+        offset2Adj = o2 + buf2.arrayOffset() + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET;
+        refObj2 = buf2.array();
+      }
+      return compareToUnsafe(buf1, o1 + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET, l1,
+          refObj2, offset2Adj, l2);
+    }
+    int end1 = o1 + l1;
+    int end2 = o2 + l2;
+    for (int i = o1, j = o2; i < end1 && j < end2; i++, j++) {
+      int a = buf1[i] & 0xFF;
+      int b = buf2.get(i) & 0xFF;
+      if (a != b) {
+        return a - b;
+      }
+    }
+    return l1 - l2;
+  }
+
   public static int compareTo(ByteBuffer buf1, int o1, int l1, byte[] buf2, int o2, int l2)
{
     if (UNSAFE_UNALIGNED) {
       long offset1Adj;
@@ -626,8 +654,8 @@ public final class ByteBufferUtils {
         offset1Adj = o1 + buf1.arrayOffset() + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET;
         refObj1 = buf1.array();
       }
-      return compareToUnsafe(refObj1, offset1Adj, l1, buf2, o2
-          + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET, l2);
+      return compareToUnsafe(refObj1, offset1Adj, l1,
+          buf2, o2 + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET, l2);
     }
     int end1 = o1 + l1;
     int end2 = o2 + l2;

http://git-wip-us.apache.org/repos/asf/hbase/blob/83c506d9/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DNS.java
----------------------------------------------------------------------
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DNS.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DNS.java
index d105a34..4b9e87f 100644
--- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DNS.java
+++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DNS.java
@@ -25,6 +25,8 @@ import org.apache.hadoop.hbase.classification.InterfaceAudience;
  * Wrapper around Hadoop's DNS class to hide reflection.
  */
 @InterfaceAudience.Private
+@edu.umd.cs.findbugs.annotations.SuppressWarnings(value="REC_CATCH_EXCEPTION",
+  justification="If exception, presume HAS_NEW_DNS_GET_DEFAULT_HOST_API false")
 public final class DNS {
   private static boolean HAS_NEW_DNS_GET_DEFAULT_HOST_API;
   private static Method GET_DEFAULT_HOST_METHOD;
@@ -35,7 +37,7 @@ public final class DNS {
           .getMethod("getDefaultHost", String.class, String.class, boolean.class);
       HAS_NEW_DNS_GET_DEFAULT_HOST_API = true;
     } catch (Exception e) {
-      HAS_NEW_DNS_GET_DEFAULT_HOST_API = false;
+      HAS_NEW_DNS_GET_DEFAULT_HOST_API = false; // FindBugs: Causes REC_CATCH_EXCEPTION.
Suppressed
     }
   }
 

http://git-wip-us.apache.org/repos/asf/hbase/blob/83c506d9/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DynamicClassLoader.java
----------------------------------------------------------------------
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DynamicClassLoader.java
b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DynamicClassLoader.java
index 2d5eb5d..214c917 100644
--- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DynamicClassLoader.java
+++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DynamicClassLoader.java
@@ -99,7 +99,9 @@ public class DynamicClassLoader extends ClassLoaderBase {
     }
   }
 
-  private void initTempDir(final Configuration conf) {
+  // FindBugs: Making synchronized to avoid IS2_INCONSISTENT_SYNC complaints about
+  // remoteDirFs and jarModifiedTime being part synchronized protected.
+  private synchronized void initTempDir(final Configuration conf) {
     jarModifiedTime = new HashMap<String, Long>();
     String localDirPath = conf.get(
       LOCAL_DIR_KEY, DEFAULT_LOCAL_DIR) + DYNAMIC_JARS_DIR;

http://git-wip-us.apache.org/repos/asf/hbase/blob/83c506d9/hbase-common/src/main/java/org/apache/hadoop/hbase/util/UnsafeAccess.java
----------------------------------------------------------------------
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/UnsafeAccess.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/UnsafeAccess.java
index e72c9f0..af2632b 100644
--- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/UnsafeAccess.java
+++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/UnsafeAccess.java
@@ -34,6 +34,8 @@ import sun.nio.ch.DirectBuffer;
 
 @InterfaceAudience.Private
 @InterfaceStability.Evolving
+@edu.umd.cs.findbugs.annotations.SuppressWarnings(value="REC_CATCH_EXCEPTION",
+  justification="If exception, presume unaligned")
 public final class UnsafeAccess {
 
   private static final Log LOG = LogFactory.getLog(UnsafeAccess.class);
@@ -51,7 +53,6 @@ public final class UnsafeAccess {
   // copyMemory method. A limit is imposed to allow for safepoint polling
   // during a large copy
   static final long UNSAFE_COPY_THRESHOLD = 1024L * 1024L;
-
   static {
     theUnsafe = (Unsafe) AccessController.doPrivileged(new PrivilegedAction<Object>()
{
       @Override
@@ -76,7 +77,7 @@ public final class UnsafeAccess {
         m.setAccessible(true);
         unaligned = (boolean) m.invoke(null);
       } catch (Exception e) {
-        unaligned = false;
+        unaligned = false; // FindBugs: Causes REC_CATCH_EXCEPTION. Suppressed.
       }
     } else{
       BYTE_ARRAY_BASE_OFFSET = -1;

http://git-wip-us.apache.org/repos/asf/hbase/blob/83c506d9/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestByteBufferUtils.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestByteBufferUtils.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestByteBufferUtils.java
index 2403c82..8ef07d2 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestByteBufferUtils.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestByteBufferUtils.java
@@ -387,6 +387,15 @@ public class TestByteBufferUtils {
     assertTrue(ByteBufferUtils.compareTo(bb1, 0, bb1.remaining(), bb2, 0, bb2.remaining())
< 0);
     bb2.put(6, (byte) 4);
     assertTrue(ByteBufferUtils.compareTo(bb1, 0, bb1.remaining(), bb2, 0, bb2.remaining())
> 0);
+    // Assert reverse comparing BB and bytearray works.
+    ByteBuffer bb3 = ByteBuffer.allocate(135);
+    fillBB(bb3, (byte)0);
+    byte[] b3 = new byte[135];
+    fillArray(b3, (byte)1);
+    int result = ByteBufferUtils.compareTo(b3, 0, b3.length, bb3, 0, bb3.remaining());
+    assertTrue(result > 0);
+    result = ByteBufferUtils.compareTo(bb3, 0, bb3.remaining(), b3, 0, b3.length);
+    assertTrue(result < 0);
   }
 
   private static void fillBB(ByteBuffer bb, byte b) {


Mime
View raw message