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 9cbd81b678ae..f5a1e5f635b9 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; @@ -80,41 +81,72 @@ 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); + } + 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); + } } 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); + } } 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); + } } 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 CompareOperator 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 3d052fc68aae..f0fa6845035e 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 @@ -117,7 +117,7 @@ public boolean filterAllRemaining() { } @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 73f859f76aea..40fe539f9fa9 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 @@ -56,7 +56,7 @@ public FamilyFilter(final CompareOperator op, final ByteArrayComparable familyCo } @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 e11e2ad4857e..1b5032fa3c5b 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 @@ -53,7 +53,7 @@ public QualifierFilter(final CompareOperator op, final ByteArrayComparable quali } @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 017185670be2..10b42ac7ee58 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 @@ -67,7 +67,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 43b3316db779..a804a6590460 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 @@ -147,7 +147,7 @@ public boolean filterRowKey(Cell cell) throws IOException { } @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) { @@ -168,9 +168,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 0056a3aff0da..ddb591ab7218 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 @@ -54,7 +54,7 @@ public ValueFilter(final CompareOperator valueCompareOp, } @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..cbb9f3415c0e --- /dev/null +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/filter/TestFiltersWithComparatorException.java @@ -0,0 +1,204 @@ +/* + * 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 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.SmallTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.Assert; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category(SmallTests.class) +public class TestFiltersWithComparatorException { + + /** + * Tests that filters which take a ByteArrayComparable comparator handle runtime exceptions in the + * comparator layer, see HBASE-29672 + */ + + byte[] cf = Bytes.toBytes("cf"); + byte[] row = Bytes.toBytes("row1"); + byte[] cq = Bytes.toBytes("q"); + long ts = 12345L; + byte[] value = Bytes.toBytes("value"); + 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 + List filterFunctionsToTest = Arrays.asList((Filter filter) -> { + filter.filterRowKey(testCell); + }, (Filter filter) -> { + filter.filterAllRemaining(); + }, (Filter filter) -> { + filter.filterCell(testCell); + }, (Filter filter) -> { + filter.filterRowCells(new ArrayList<>(Collections.singletonList(testCell))); + }, (Filter filter) -> { + 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 compareToInvokations = 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) { + compareToInvokations++; + throw new RuntimeException("comparator runtime exception"); + } + + @Override + public int compareTo(ByteBuffer value, int offset, int length) { + compareToInvokations++; + throw new RuntimeException("comparator runtime exception"); + } + + @Override + public int compareTo(byte[] value) { + compareToInvokations++; + 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.compareToInvokations; + boolean ioExceptionThrown = false; + try { + filterFunction.run(filter); + } catch (HBaseIOException e) { + ioExceptionThrown = true; + } catch (IOException ignored) { + } + if (invocationsBefore != badComparator.compareToInvokations) { + Assert.assertTrue("IOException should have been thrown", ioExceptionThrown); + } + } + if (badComparator.compareToInvokations == 0) { + Assert + .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); + } + +}