Repository: hbase Updated Branches: refs/heads/branch-1.2 9bd76426f -> 6d1082f62
HBASE-17187 DoNotRetryExceptions from coprocessors should bubble up to the application Project: http://git-wip-us.apache.org/repos/asf/hbase/repo Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/6d1082f6 Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/6d1082f6 Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/6d1082f6 Branch: refs/heads/branch-1.2 Commit: 6d1082f626d7bdc3eb246cb0b9976c84501b16cb Parents: a07b968 Author: Enis Soztutar <e...@apache.org> Authored: Mon Feb 6 12:00:05 2017 -0800 Committer: Enis Soztutar <e...@apache.org> Committed: Mon Feb 6 16:11:53 2017 -0800 ---------------------------------------------------------------------- .../hadoop/hbase/client/ClientScanner.java | 20 +++++- .../hbase/regionserver/RSRpcServices.java | 12 +++- .../hadoop/hbase/client/TestFromClientSide.java | 76 +++++++++++++++++++- 3 files changed, 103 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hbase/blob/6d1082f6/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java ---------------------------------------------------------------------- diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java index 944f44e..053814c 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java @@ -401,6 +401,9 @@ public class ClientScanner extends AbstractClientScanner { // it doesn't tell us otherwise. We rely on the size or count of results boolean serverHasMoreResults = false; boolean allResultsSkipped = false; + // Even if we are retrying due to UnknownScannerException, ScannerResetException, etc. we should + // make sure that we are not retrying indefinitely. + int retriesLeft = getRetries(); do { allResultsSkipped = false; try { @@ -425,8 +428,18 @@ public class ClientScanner extends AbstractClientScanner { // An exception was thrown which makes any partial results that we were collecting // invalid. The scanner will need to be reset to the beginning of a row. clearPartialResults(); - // DNRIOEs are thrown to make us break out of retries. Some types of DNRIOEs want us - // to reset the scanner and come back in again. + + // Unfortunately, DNRIOE is used in two different semantics. + // (1) The first is to close the client scanner and bubble up the exception all the way + // to the application. This is preferred when the exception is really un-recoverable + // (like CorruptHFileException, etc). Plain DoNotRetryIOException also falls into this + // bucket usually. + // (2) Second semantics is to close the current region scanner only, but continue the + // client scanner by overriding the exception. This is usually UnknownScannerException, + // OutOfOrderScannerNextException, etc where the region scanner has to be closed, but the + // application-level ClientScanner has to continue without bubbling up the exception to + // the client. See RSRpcServices to see how it throws DNRIOE's. + // See also: HBASE-16604, HBASE-17187 // If exception is any but the list below throw it back to the client; else setup // the scanner and retry. @@ -438,6 +451,9 @@ public class ClientScanner extends AbstractClientScanner { e instanceof ScannerResetException) { // Pass. It is easier writing the if loop test as list of what is allowed rather than // as a list of what is not allowed... so if in here, it means we do not throw. + if (retriesLeft-- <= 0) { + throw e; // no more retries + } } else { throw e; } http://git-wip-us.apache.org/repos/asf/hbase/blob/6d1082f6/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java index 46a8120..1d454a1 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java @@ -2663,7 +2663,17 @@ public class RSRpcServices implements HBaseRPCErrorHandler, // row that the client has last seen. closeScanner(region, scanner, scannerName); - // rethrow DoNotRetryIOException. This can avoid the retry in ClientScanner. + // If it is a DoNotRetryIOException already, throw as it is. Unfortunately, DNRIOE is + // used in two different semantics. + // (1) The first is to close the client scanner and bubble up the exception all the way + // to the application. This is preferred when the exception is really un-recoverable + // (like CorruptHFileException, etc). Plain DoNotRetryIOException also falls into this + // bucket usually. + // (2) Second semantics is to close the current region scanner only, but continue the + // client scanner by overriding the exception. This is usually UnknownScannerException, + // OutOfOrderScannerNextException, etc where the region scanner has to be closed, but the + // application-level ClientScanner has to continue without bubbling up the exception to + // the client. See ClientScanner code to see how it deals with these special exceptions. if (e instanceof DoNotRetryIOException) { throw e; } http://git-wip-us.apache.org/repos/asf/hbase/blob/6d1082f6/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java index 0b2ba83..e8c6776 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java @@ -41,6 +41,7 @@ import java.util.UUID; import java.util.concurrent.Callable; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; @@ -52,6 +53,7 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.Abortable; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CellUtil; +import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HBaseTestingUtility; import org.apache.hadoop.hbase.HColumnDescriptor; import org.apache.hadoop.hbase.HConstants; @@ -70,6 +72,7 @@ import org.apache.hadoop.hbase.coprocessor.CoprocessorHost; import org.apache.hadoop.hbase.coprocessor.MultiRowMutationEndpoint; import org.apache.hadoop.hbase.coprocessor.ObserverContext; import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment; +import org.apache.hadoop.hbase.exceptions.ScannerResetException; import org.apache.hadoop.hbase.filter.BinaryComparator; import org.apache.hadoop.hbase.filter.CompareFilter; import org.apache.hadoop.hbase.filter.CompareFilter.CompareOp; @@ -599,6 +602,15 @@ public class TestFromClientSide { */ public static class ExceptionInReseekRegionObserver extends BaseRegionObserver { static AtomicLong reqCount = new AtomicLong(0); + static AtomicBoolean isDoNotRetry = new AtomicBoolean(false); // whether to throw DNRIOE + static AtomicBoolean throwOnce = new AtomicBoolean(true); // whether to only throw once + + static void reset() { + reqCount.set(0); + isDoNotRetry.set(false); + throwOnce.set(true); + } + class MyStoreScanner extends StoreScanner { public MyStoreScanner(Store store, ScanInfo scanInfo, Scan scan, NavigableSet<byte[]> columns, long readPt) throws IOException { @@ -614,8 +626,13 @@ public class TestFromClientSide { newScanners.add(new DelegatingKeyValueScanner(scanner) { @Override public boolean reseek(Cell key) throws IOException { - if (reqCount.incrementAndGet() == 1) { - throw new IOException("Injected exception"); + reqCount.incrementAndGet(); + if (!throwOnce.get()|| reqCount.get() == 1) { + if (isDoNotRetry.get()) { + throw new DoNotRetryIOException("Injected exception"); + } else { + throw new IOException("Injected exception"); + } } return super.reseek(key); } @@ -649,6 +666,8 @@ public class TestFromClientSide { HTableDescriptor htd = TEST_UTIL.createTableDescriptor(name, FAMILY); htd.addCoprocessor(ExceptionInReseekRegionObserver.class.getName()); TEST_UTIL.getHBaseAdmin().createTable(htd); + ExceptionInReseekRegionObserver.reset(); + ExceptionInReseekRegionObserver.throwOnce.set(true); // throw exceptions only once try (Table t = TEST_UTIL.getConnection().getTable(name)) { int rowCount = TEST_UTIL.loadTable(t, FAMILY, false); TEST_UTIL.getHBaseAdmin().flush(name); @@ -658,6 +677,59 @@ public class TestFromClientSide { assertTrue(ExceptionInReseekRegionObserver.reqCount.get() > 0); } + /** + * Tests the case where a coprocessor throws a DoNotRetryIOException in the scan. The expectation + * is that the exception will bubble up to the client scanner instead of being retried. + */ + @Test (timeout = 180000) + public void testScannerThrowsExceptionWhenCoprocessorThrowsDNRIOE() + throws IOException, InterruptedException { + TEST_UTIL.getConfiguration().setBoolean("hbase.client.log.scanner.activity", true); + TableName name = TableName.valueOf("testClientScannerIsNotRetriedWhenCoprocessorThrowsDNRIOE"); + + HTableDescriptor htd = TEST_UTIL.createTableDescriptor(name, FAMILY); + htd.addCoprocessor(ExceptionInReseekRegionObserver.class.getName()); + TEST_UTIL.getHBaseAdmin().createTable(htd); + ExceptionInReseekRegionObserver.reset(); + ExceptionInReseekRegionObserver.isDoNotRetry.set(true); + try (Table t = TEST_UTIL.getConnection().getTable(name)) { + TEST_UTIL.loadTable(t, FAMILY, false); + TEST_UTIL.getHBaseAdmin().flush(name); + countRows(t, new Scan().addColumn(FAMILY, FAMILY)); + fail("Should have thrown an exception"); + } catch (RuntimeException expected) { + // expected + } + assertTrue(ExceptionInReseekRegionObserver.reqCount.get() > 0); + } + + /** + * Tests the case where a coprocessor throws a regular IOException in the scan. The expectation + * is that the we will keep on retrying, but fail after the retries are exhausted instead of + * retrying indefinitely. + */ + @Test (timeout = 180000) + public void testScannerFailsAfterRetriesWhenCoprocessorThrowsIOE() + throws IOException, InterruptedException { + TEST_UTIL.getConfiguration().setBoolean("hbase.client.log.scanner.activity", true); + TableName name = TableName.valueOf("testScannerFailsAfterRetriesWhenCoprocessorThrowsIOE"); + TEST_UTIL.getConfiguration().setInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER, 3); + HTableDescriptor htd = TEST_UTIL.createTableDescriptor(name, FAMILY); + htd.addCoprocessor(ExceptionInReseekRegionObserver.class.getName()); + TEST_UTIL.getHBaseAdmin().createTable(htd); + ExceptionInReseekRegionObserver.reset(); + ExceptionInReseekRegionObserver.throwOnce.set(false); // throw exceptions in every retry + try (Table t = TEST_UTIL.getConnection().getTable(name)) { + TEST_UTIL.loadTable(t, FAMILY, false); + TEST_UTIL.getHBaseAdmin().flush(name); + countRows(t, new Scan().addColumn(FAMILY, FAMILY)); + fail("Should have thrown an exception"); + } catch (RuntimeException expected) { + // expected + } + assertTrue(ExceptionInReseekRegionObserver.reqCount.get() >= 3); + } + /* * @param key * @return Scan with RowFilter that does LESS than passed key.
