diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ColumnValueFilter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ColumnValueFilter.java index 1991100d0daa..0c0c0ed60be8 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ColumnValueFilter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ColumnValueFilter.java @@ -121,12 +121,16 @@ public ReturnCode filterCell(Cell c) throws IOException { * @return true means cell should be filtered out, included otherwise. */ private boolean compareValue(final CompareOperator op, final ByteArrayComparable comparator, - final Cell cell) { + final Cell cell) throws IOException { if (op == CompareOperator.NO_OP) { return true; } - int compareResult = PrivateCellUtil.compareValue(cell, comparator); - return CompareFilter.compare(op, compareResult); + try { + int compareResult = PrivateCellUtil.compareValue(cell, comparator); + return CompareFilter.compare(op, compareResult); + } catch (RuntimeException e) { + throw CompareFilter.wrapInHBaseIOException(e, comparator); + } } /** diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/CompareFilter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/CompareFilter.java index 9ed9c526bb96..3219563e3c6e 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/CompareFilter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/CompareFilter.java @@ -22,6 +22,7 @@ import java.util.Objects; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CompareOperator; +import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.PrivateCellUtil; import org.apache.hadoop.hbase.util.Bytes; import org.apache.yetus.audience.InterfaceAudience; @@ -123,6 +124,21 @@ public boolean filterRowKey(Cell cell) throws IOException { return false; } + /** + * RuntimeException when applying a comparator indicates a code bug or misconfigured + * filter/comparator, we wrap it in `HBaseIOException` to provide a clear exception message/stack + * trace and prevent propagating a runtime exception up the call stack (which would lead to + * unexpected throwable at RpcServer layer and a complicated unclear remote exception on the + * client) + */ + public static HBaseIOException wrapInHBaseIOException(RuntimeException e, + ByteArrayComparable comparator) { + String msg = + String.format("Runtime exception occurred when applying comparator %s during filtering", + comparator.getClass().getSimpleName()); + return new HBaseIOException(msg, e); + } + /** * @deprecated Since 2.0.0. Will be removed in 3.0.0. Use * {@link #compareRow(CompareOperator, ByteArrayComparable, Cell)} @@ -138,12 +154,16 @@ protected boolean compareRow(final CompareOp compareOp, final ByteArrayComparabl } protected boolean compareRow(final CompareOperator op, final ByteArrayComparable comparator, - final Cell cell) { + final Cell cell) throws IOException { if (op == CompareOperator.NO_OP) { return true; } - int compareResult = PrivateCellUtil.compareRow(cell, comparator); - return compare(op, compareResult); + try { + int compareResult = PrivateCellUtil.compareRow(cell, comparator); + return compare(op, compareResult); + } catch (RuntimeException e) { + throw wrapInHBaseIOException(e, comparator); + } } /** @@ -161,12 +181,16 @@ protected boolean compareFamily(final CompareOp compareOp, final ByteArrayCompar } protected boolean compareFamily(final CompareOperator op, final ByteArrayComparable comparator, - final Cell cell) { + final Cell cell) throws IOException { if (op == CompareOperator.NO_OP) { return true; } - int compareResult = PrivateCellUtil.compareFamily(cell, comparator); - return compare(op, compareResult); + try { + int compareResult = PrivateCellUtil.compareFamily(cell, comparator); + return compare(op, compareResult); + } catch (RuntimeException e) { + throw wrapInHBaseIOException(e, comparator); + } } /** @@ -185,13 +209,17 @@ protected boolean compareQualifier(final CompareOp compareOp, } protected boolean compareQualifier(final CompareOperator op, final ByteArrayComparable comparator, - final Cell cell) { + final Cell cell) throws IOException { // We do not call through to the non-deprecated method for perf reasons. if (op == CompareOperator.NO_OP) { return true; } - int compareResult = PrivateCellUtil.compareQualifier(cell, comparator); - return compare(op, compareResult); + try { + int compareResult = PrivateCellUtil.compareQualifier(cell, comparator); + return compare(op, compareResult); + } catch (RuntimeException e) { + throw wrapInHBaseIOException(e, comparator); + } } /** @@ -210,12 +238,16 @@ protected boolean compareValue(final CompareOp compareOp, final ByteArrayCompara } protected boolean compareValue(final CompareOperator op, final ByteArrayComparable comparator, - final Cell cell) { + final Cell cell) throws IOException { if (op == CompareOperator.NO_OP) { return true; } - int compareResult = PrivateCellUtil.compareValue(cell, comparator); - return compare(op, compareResult); + try { + int compareResult = PrivateCellUtil.compareValue(cell, comparator); + return compare(op, compareResult); + } catch (RuntimeException e) { + throw wrapInHBaseIOException(e, comparator); + } } static boolean compare(final CompareOp op, int compareResult) { diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/DependentColumnFilter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/DependentColumnFilter.java index be59929dfc43..27d48c02a5c1 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/DependentColumnFilter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/DependentColumnFilter.java @@ -138,12 +138,12 @@ public boolean filterAllRemaining() { @Deprecated @Override - public ReturnCode filterKeyValue(final Cell c) { + public ReturnCode filterKeyValue(final Cell c) throws IOException { return filterCell(c); } @Override - public ReturnCode filterCell(final Cell c) { + public ReturnCode filterCell(final Cell c) throws IOException { // Check if the column and qualifier match if (!CellUtil.matchingColumn(c, this.columnFamily, this.columnQualifier)) { // include non-matches for the time being, they'll be discarded afterwards diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FamilyFilter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FamilyFilter.java index a5c6c8d266a1..d2f0344732e8 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FamilyFilter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FamilyFilter.java @@ -70,12 +70,12 @@ public FamilyFilter(final CompareOperator op, final ByteArrayComparable familyCo @Deprecated @Override - public ReturnCode filterKeyValue(final Cell c) { + public ReturnCode filterKeyValue(final Cell c) throws IOException { return filterCell(c); } @Override - public ReturnCode filterCell(final Cell c) { + public ReturnCode filterCell(final Cell c) throws IOException { int familyLength = c.getFamilyLength(); if (familyLength > 0) { if (compareFamily(getCompareOperator(), this.comparator, c)) { diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/QualifierFilter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/QualifierFilter.java index 328c7fd57c6c..6ca8c91cc699 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/QualifierFilter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/QualifierFilter.java @@ -67,12 +67,12 @@ public QualifierFilter(final CompareOperator op, final ByteArrayComparable quali @Deprecated @Override - public ReturnCode filterKeyValue(final Cell c) { + public ReturnCode filterKeyValue(final Cell c) throws IOException { return filterCell(c); } @Override - public ReturnCode filterCell(final Cell c) { + public ReturnCode filterCell(final Cell c) throws IOException { if (compareQualifier(getCompareOperator(), this.comparator, c)) { return ReturnCode.SKIP; } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/RowFilter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/RowFilter.java index cdebf0a15dcc..c34de1b423bf 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/RowFilter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/RowFilter.java @@ -86,7 +86,7 @@ public ReturnCode filterCell(final Cell v) { } @Override - public boolean filterRowKey(Cell firstRowCell) { + public boolean filterRowKey(Cell firstRowCell) throws IOException { if (compareRow(getCompareOperator(), this.comparator, firstRowCell)) { this.filterOutRow = true; } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/SingleColumnValueFilter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/SingleColumnValueFilter.java index 63233e772a15..17b048dc15be 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/SingleColumnValueFilter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/SingleColumnValueFilter.java @@ -214,12 +214,12 @@ public boolean filterRowKey(Cell cell) throws IOException { @Deprecated @Override - public ReturnCode filterKeyValue(final Cell c) { + public ReturnCode filterKeyValue(final Cell c) throws IOException { return filterCell(c); } @Override - public ReturnCode filterCell(final Cell c) { + public ReturnCode filterCell(final Cell c) throws IOException { // System.out.println("REMOVE KEY=" + keyValue.toString() + ", value=" + // Bytes.toString(keyValue.getValue())); if (this.matchedColumn) { @@ -240,9 +240,13 @@ public ReturnCode filterCell(final Cell c) { return ReturnCode.INCLUDE; } - private boolean filterColumnValue(final Cell cell) { - int compareResult = PrivateCellUtil.compareValue(cell, this.comparator); - return CompareFilter.compare(this.op, compareResult); + private boolean filterColumnValue(final Cell cell) throws IOException { + try { + int compareResult = PrivateCellUtil.compareValue(cell, this.comparator); + return CompareFilter.compare(this.op, compareResult); + } catch (RuntimeException e) { + throw CompareFilter.wrapInHBaseIOException(e, this.comparator); + } } @Override diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ValueFilter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ValueFilter.java index 5339530923fa..23b8c56f0121 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ValueFilter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ValueFilter.java @@ -67,12 +67,12 @@ public ValueFilter(final CompareOperator valueCompareOp, @Deprecated @Override - public ReturnCode filterKeyValue(final Cell c) { + public ReturnCode filterKeyValue(final Cell c) throws IOException { return filterCell(c); } @Override - public ReturnCode filterCell(final Cell c) { + public ReturnCode filterCell(final Cell c) throws IOException { if (compareValue(getCompareOperator(), this.comparator, c)) { return ReturnCode.SKIP; } diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/filter/TestFiltersWithComparatorException.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/filter/TestFiltersWithComparatorException.java new file mode 100644 index 000000000000..4e1f37ce24df --- /dev/null +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/filter/TestFiltersWithComparatorException.java @@ -0,0 +1,199 @@ +/* + * 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.filter; + +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.CompareOperator; +import org.apache.hadoop.hbase.HBaseIOException; +import org.apache.hadoop.hbase.KeyValue; +import org.apache.hadoop.hbase.testclassification.FilterTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +@Tag(SmallTests.TAG) +@Tag(FilterTests.TAG) +public class TestFiltersWithComparatorException { + + /** + * Tests that filters which take a ByteArrayComparable comparator handle runtime exceptions in the + * comparator layer, see HBASE-29672 + */ + + final byte[] cf = Bytes.toBytes("cf"); + final byte[] row = Bytes.toBytes("row1"); + final byte[] cq = Bytes.toBytes("q"); + final long ts = 12345L; + final byte[] value = Bytes.toBytes("value"); + final Cell testCell = new KeyValue(row, cf, cq, ts, value); + + @FunctionalInterface + interface FilterFunctionThrowable { + void run(Filter filter) throws IOException; + } + + // Every filterX method that Filter implements to test + final List filterFunctionsToTest = + Arrays.asList(filter -> filter.filterRowKey(testCell), Filter::filterAllRemaining, + filter -> filter.filterCell(testCell), + filter -> filter.filterRowCells(new ArrayList<>(Collections.singletonList(testCell))), + Filter::filterRow); + + /** + * Comparator which throws RuntimeException for every `compareTo` method that + * `ByteArrayComparable` implements + keeps a counter for number of compareTo invocations / + * RuntimeExceptions thrown + */ + static class BadComparator extends ByteArrayComparable { + + public int compareToInvocations = 0; + + public BadComparator() { + super(new byte[1]); + } + + @Override + public byte[] toByteArray() { + return new byte[1]; + } + + @Override + public int compareTo(byte[] value, int offset, int length) { + compareToInvocations++; + throw new RuntimeException("comparator runtime exception"); + } + + @Override + public int compareTo(ByteBuffer value, int offset, int length) { + compareToInvocations++; + throw new RuntimeException("comparator runtime exception"); + } + + @Override + public int compareTo(byte[] value) { + compareToInvocations++; + throw new RuntimeException("comparator runtime exception"); + } + + } + + /** + * Verifies that a given {@link Filter} correctly triggers comparator logic and wraps any runtime + * exceptions happening at comparison time in {@link org.apache.hadoop.hbase.HBaseIOException} + *

+ * This method runs a set of predefined filter functions against the provided filter instance and + * checks that if a comparator invocation occurs which throws a RuntimeException, it gets wrapped + * in a {@code HBaseIOException} by the filter implementation The test fails if: + *

+ * @param filter the filter instance under test + * @param badComparator the comparator the filter was constructed with + **/ + private void testFilter(Filter filter, BadComparator badComparator) { + for (FilterFunctionThrowable filterFunction : filterFunctionsToTest) { + int invocationsBefore = badComparator.compareToInvocations; + boolean ioExceptionThrown = false; + try { + filterFunction.run(filter); + } catch (HBaseIOException e) { + ioExceptionThrown = true; + } catch (IOException ignored) { + } + if (invocationsBefore != badComparator.compareToInvocations) { + assertTrue(ioExceptionThrown, "IOException should have been thrown"); + } + } + if (badComparator.compareToInvocations == 0) { + fail(String.format("Filter %s never invoked the comparator for any of the functions tested - " + + "intended behavior was not tested, this is not expected", filter.getClass().getName())); + } + } + + @Test + public void testColumnValueFilter() { + BadComparator comparator = new BadComparator(); + ColumnValueFilter columnValueFilter = + new ColumnValueFilter(cf, cq, CompareOperator.EQUAL, comparator); + testFilter(columnValueFilter, comparator); + } + + @Test + public void testRowFilter() { + BadComparator comparator = new BadComparator(); + RowFilter rowFilter = new RowFilter(CompareOperator.EQUAL, comparator); + testFilter(rowFilter, comparator); + } + + @Test + public void testDependentColumnFilter() { + BadComparator comparator = new BadComparator(); + DependentColumnFilter filter = + new DependentColumnFilter(cf, cq, false, CompareOperator.EQUAL, comparator); + testFilter(filter, comparator); + } + + @Test + public void testFamilyFilter() { + BadComparator comparator = new BadComparator(); + FamilyFilter filter = new FamilyFilter(CompareOperator.EQUAL, comparator); + testFilter(filter, comparator); + } + + @Test + public void testQualifierFilter() { + BadComparator comparator = new BadComparator(); + QualifierFilter filter = new QualifierFilter(CompareOperator.EQUAL, comparator); + testFilter(filter, comparator); + } + + @Test + public void testSingleColumnValueExcludeFilter() { + BadComparator comparator = new BadComparator(); + SingleColumnValueExcludeFilter filter = + new SingleColumnValueExcludeFilter(cf, cq, CompareOperator.EQUAL, comparator); + testFilter(filter, comparator); + } + + @Test + public void testSingleColumnValueFilter() { + BadComparator comparator = new BadComparator(); + SingleColumnValueFilter filter = + new SingleColumnValueFilter(cf, cq, CompareOperator.EQUAL, comparator); + testFilter(filter, comparator); + } + + @Test + public void testValueFilter() { + BadComparator comparator = new BadComparator(); + ValueFilter filter = new ValueFilter(CompareOperator.EQUAL, comparator); + testFilter(filter, comparator); + } + +}