From fa6dc9c44e28aa608d5204db814e8442b95eb125 Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Wed, 6 May 2015 18:13:46 +0200 Subject: [PATCH] HBASE-13634 Avoid unsafe reference equality checks to EMPTY byte[] Add comment Signed-off-by: stack --- .../hbase/regionserver/ScanQueryMatcher.java | 5 ++-- .../regionserver/StripeMultiFileWriter.java | 22 ++++++++------ .../regionserver/StripeStoreFileManager.java | 29 ++++++++++++++----- .../hbase/util/RegionSplitCalculator.java | 1 + 4 files changed, 38 insertions(+), 19 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java index 86390da945e..e67e29252e9 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java @@ -20,6 +20,7 @@ package org.apache.hadoop.hbase.regionserver; import java.io.IOException; +import java.util.Arrays; import java.util.NavigableSet; import org.apache.hadoop.hbase.KeyValue.Type; @@ -474,7 +475,7 @@ public class ScanQueryMatcher { // dropDeletesFromRow is leq current kv, we start dropping deletes and reset // dropDeletesFromRow; thus the 2nd "if" starts to apply. if ((dropDeletesFromRow != null) - && ((dropDeletesFromRow == HConstants.EMPTY_START_ROW) + && (Arrays.equals(dropDeletesFromRow, HConstants.EMPTY_START_ROW) || (Bytes.compareTo(row, offset, length, dropDeletesFromRow, 0, dropDeletesFromRow.length) >= 0))) { retainDeletesInOutput = false; @@ -484,7 +485,7 @@ public class ScanQueryMatcher { // drop-deletes range. When dropDeletesToRow is leq current kv, we stop dropping deletes, // and reset dropDeletesToRow so that we don't do any more compares. if ((dropDeletesFromRow == null) - && (dropDeletesToRow != null) && (dropDeletesToRow != HConstants.EMPTY_END_ROW) + && (dropDeletesToRow != null) && !Arrays.equals(dropDeletesToRow, HConstants.EMPTY_END_ROW) && (Bytes.compareTo(row, offset, length, dropDeletesToRow, 0, dropDeletesToRow.length) >= 0)) { retainDeletesInOutput = true; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeMultiFileWriter.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeMultiFileWriter.java index 26c9470fb6d..99f4a6b42e2 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeMultiFileWriter.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeMultiFileWriter.java @@ -19,6 +19,7 @@ package org.apache.hadoop.hbase.regionserver; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -116,7 +117,7 @@ public abstract class StripeMultiFileWriter implements Compactor.CellSink { */ protected void sanityCheckLeft( byte[] left, Cell cell) throws IOException { - if (StripeStoreFileManager.OPEN_KEY != left && + if (!Arrays.equals(StripeStoreFileManager.OPEN_KEY, left) && comparator.compareRows(cell, left, 0, left.length) < 0) { String error = "The first row is lower than the left boundary of [" + Bytes.toString(left) + "]: [" + Bytes.toString(cell.getRowArray(), cell.getRowOffset(), cell.getRowLength()) @@ -132,7 +133,7 @@ public abstract class StripeMultiFileWriter implements Compactor.CellSink { */ protected void sanityCheckRight( byte[] right, Cell cell) throws IOException { - if (StripeStoreFileManager.OPEN_KEY != right && + if (!Arrays.equals(StripeStoreFileManager.OPEN_KEY, right) && comparator.compareRows(cell, right, 0, right.length) >= 0) { String error = "The last row is higher or equal than the right boundary of [" + Bytes.toString(right) + "]: [" @@ -178,10 +179,14 @@ public abstract class StripeMultiFileWriter implements Compactor.CellSink { // must match some target boundaries, let's find them. assert (majorRangeFrom == null) == (majorRangeTo == null); if (majorRangeFrom != null) { - majorRangeFromIndex = (majorRangeFrom == StripeStoreFileManager.OPEN_KEY) ? 0 - : Collections.binarySearch(this.boundaries, majorRangeFrom, Bytes.BYTES_COMPARATOR); - majorRangeToIndex = (majorRangeTo == StripeStoreFileManager.OPEN_KEY) ? boundaries.size() - : Collections.binarySearch(this.boundaries, majorRangeTo, Bytes.BYTES_COMPARATOR); + majorRangeFromIndex = Arrays.equals(majorRangeFrom, StripeStoreFileManager.OPEN_KEY) + ? 0 + : Collections.binarySearch(boundaries, majorRangeFrom, + Bytes.BYTES_COMPARATOR); + majorRangeToIndex = Arrays.equals(majorRangeTo, StripeStoreFileManager.OPEN_KEY) + ? boundaries.size() + : Collections.binarySearch(boundaries, majorRangeTo, + Bytes.BYTES_COMPARATOR); if (this.majorRangeFromIndex < 0 || this.majorRangeToIndex < 0) { throw new IOException("Major range does not match writer boundaries: [" + Bytes.toString(majorRangeFrom) + "] [" + Bytes.toString(majorRangeTo) + "]; from " @@ -204,9 +209,8 @@ public abstract class StripeMultiFileWriter implements Compactor.CellSink { } private boolean isCellAfterCurrentWriter(Cell cell) { - return ((currentWriterEndKey != StripeStoreFileManager.OPEN_KEY) && - (comparator.compareRows(cell, - currentWriterEndKey, 0, currentWriterEndKey.length) >= 0)); + return !Arrays.equals(currentWriterEndKey, StripeStoreFileManager.OPEN_KEY) && + (comparator.compareRows(cell, currentWriterEndKey, 0, currentWriterEndKey.length) >= 0); } @Override diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreFileManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreFileManager.java index 0f0301876bb..4481a5f4f0a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreFileManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreFileManager.java @@ -505,6 +505,7 @@ public class StripeStoreFileManager * Checks whether the key is invalid (e.g. from an L0 file, or non-stripe-compacted files). */ private static final boolean isInvalid(byte[] key) { + // No need to use Arrays.equals because INVALID_KEY is null return key == INVALID_KEY; } @@ -536,8 +537,10 @@ public class StripeStoreFileManager * Finds the stripe index for the stripe containing a row provided externally for get/scan. */ private final int findStripeForRow(byte[] row, boolean isStart) { - if (isStart && row == HConstants.EMPTY_START_ROW) return 0; - if (!isStart && row == HConstants.EMPTY_END_ROW) return state.stripeFiles.size() - 1; + if (isStart && Arrays.equals(row, HConstants.EMPTY_START_ROW)) return 0; + if (!isStart && Arrays.equals(row, HConstants.EMPTY_END_ROW)) { + return state.stripeFiles.size() - 1; + } // If there's an exact match below, a stripe ends at "row". Stripe right boundary is // exclusive, so that means the row is in the next stripe; thus, we need to add one to index. // If there's no match, the return value of binarySearch is (-(insertion point) - 1), where @@ -559,15 +562,25 @@ public class StripeStoreFileManager private byte[] startOf(StoreFile sf) { - byte[] result = this.fileStarts.get(sf); - return result == null ? sf.getMetadataValue(STRIPE_START_KEY) - : (result == INVALID_KEY_IN_MAP ? INVALID_KEY : result); + byte[] result = fileStarts.get(sf); + + // result and INVALID_KEY_IN_MAP are compared _only_ by reference on purpose here as the latter + // serves only as a marker and is not to be confused with other empty byte arrays. + // See Javadoc of INVALID_KEY_IN_MAP for more information + return (result == null) + ? sf.getMetadataValue(STRIPE_START_KEY) + : result == INVALID_KEY_IN_MAP ? INVALID_KEY : result; } private byte[] endOf(StoreFile sf) { - byte[] result = this.fileEnds.get(sf); - return result == null ? sf.getMetadataValue(STRIPE_END_KEY) - : (result == INVALID_KEY_IN_MAP ? INVALID_KEY : result); + byte[] result = fileEnds.get(sf); + + // result and INVALID_KEY_IN_MAP are compared _only_ by reference on purpose here as the latter + // serves only as a marker and is not to be confused with other empty byte arrays. + // See Javadoc of INVALID_KEY_IN_MAP for more information + return (result == null) + ? sf.getMetadataValue(STRIPE_END_KEY) + : result == INVALID_KEY_IN_MAP ? INVALID_KEY : result; } /** diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionSplitCalculator.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionSplitCalculator.java index b904b43f4b1..eeef1aea6e9 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionSplitCalculator.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionSplitCalculator.java @@ -115,6 +115,7 @@ public class RegionSplitCalculator { byte[] start = range.getStartKey(); byte[] end = specialEndKey(range); + // No need to use Arrays.equals because ENDKEY is null if (end != ENDKEY && Bytes.compareTo(start, end) > 0) { // don't allow backwards edges LOG.debug("attempted to add backwards edge: " + Bytes.toString(start)