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, 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, 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, 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()); 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; } 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); } } - } 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 0af64cd..02b2514 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; 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); 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 extends AbstractMap } @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 extends AbstractMap } @Override + @edu.umd.cs.findbugs.annotations.SuppressWarnings(value="IT_NO_SUCH_ELEMENT", + justification="Intentional") public Entry next() { + if (!hasNext()) { + throw new NoSuchElementException(); + } return holder.entries[index++]; } 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 6e3fcaa..1add709 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 { @@ -615,6 +616,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; @@ -625,8 +653,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; 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 } } 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 localDirPath = conf.get( LOCAL_DIR_KEY, DEFAULT_LOCAL_DIR) + DYNAMIC_JARS_DIR; 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() { @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; 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) {