From 01ceb2697e232e114e909097f3b3ba7bf601cd47 Mon Sep 17 00:00:00 2001 From: Josh Elser Date: Fri, 20 Mar 2015 13:39:42 -0400 Subject: [PATCH] HBASE-13262 Observe ScanResponse.moreResults in ClientScanner. The RS already returns to the client whether or not it has additional results to be returned in a subsequent call to scan(), but the ClientScanner did not use or adhere to this value. Subsequently, this can lead to bugs around moving to the next region too early. A new method was added to ClientScanner in the name of testability. --- .../apache/hadoop/hbase/client/ClientScanner.java | 268 ++++++------ .../hbase/client/ClientSmallReversedScanner.java | 5 +- .../hadoop/hbase/client/ClientSmallScanner.java | 24 +- .../hadoop/hbase/client/ScanResultWithContext.java | 133 ++++++ .../hadoop/hbase/client/ScannerCallable.java | 28 +- .../hbase/client/ScannerCallableWithReplicas.java | 47 ++- .../hadoop/hbase/client/TestClientScanner.java | 460 +++++++++++++++++++++ .../hbase/client/TestScanResultWithContext.java | 55 +++ .../hadoop/hbase/regionserver/RSRpcServices.java | 2 + .../hadoop/hbase/client/TestSizeFailures.java | 166 ++++++++ .../hadoop/hbase/master/TestAssignmentManager.java | 9 +- .../regionserver/TestRegionServerMetrics.java | 2 + 12 files changed, 1032 insertions(+), 167 deletions(-) create mode 100644 hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScanResultWithContext.java create mode 100644 hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestClientScanner.java create mode 100644 hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScanResultWithContext.java create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSizeFailures.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 e08c1d4..332cbbc 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 @@ -87,7 +87,7 @@ public class ClientScanner extends AbstractClientScanner { private final TableName tableName; protected final int scannerTimeout; protected boolean scanMetricsPublished = false; - protected RpcRetryingCaller caller; + protected RpcRetryingCaller caller; protected RpcControllerFactory rpcControllerFactory; protected Configuration conf; //The timeout on the primary. Applicable if there are multiple replicas for a region @@ -148,7 +148,7 @@ public class ClientScanner extends AbstractClientScanner { HConstants.DEFAULT_HBASE_CLIENT_SCANNER_CACHING); } - this.caller = rpcFactory. newCaller(); + this.caller = rpcFactory. newCaller(); this.rpcControllerFactory = controllerFactory; this.conf = conf; @@ -302,8 +302,8 @@ public class ClientScanner extends AbstractClientScanner { return callable.isAnyRPCcancelled(); } - static Result[] call(ScannerCallableWithReplicas callable, - RpcRetryingCaller caller, int scannerTimeout) + ScanResultWithContext call(ScannerCallableWithReplicas callable, + RpcRetryingCaller caller, int scannerTimeout) throws IOException, RuntimeException { if (Thread.interrupted()) { throw new InterruptedIOException(); @@ -354,126 +354,7 @@ public class ClientScanner extends AbstractClientScanner { return null; } if (cache.size() == 0) { - Result[] values = null; - long remainingResultSize = maxScannerResultSize; - int countdown = this.caching; - - // We need to reset it if it's a new callable that was created - // with a countdown in nextScanner - callable.setCaching(this.caching); - // This flag is set when we want to skip the result returned. We do - // this when we reset scanner because it split under us. - boolean retryAfterOutOfOrderException = true; - do { - try { - // Server returns a null values if scanning is to stop. Else, - // returns an empty array if scanning is to go on and we've just - // exhausted current region. - values = call(callable, caller, scannerTimeout); - - // When the replica switch happens, we need to do certain operations - // again. The callable will openScanner with the right startkey - // but we need to pick up from there. Bypass the rest of the loop - // and let the catch-up happen in the beginning of the loop as it - // happens for the cases where we see exceptions. Since only openScanner - // would have happened, values would be null - if (values == null && callable.switchedToADifferentReplica()) { - this.currentRegion = callable.getHRegionInfo(); - continue; - } - retryAfterOutOfOrderException = true; - } catch (DoNotRetryIOException e) { - // 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. - if (e instanceof UnknownScannerException) { - long timeout = lastNext + scannerTimeout; - // If we are over the timeout, throw this exception to the client wrapped in - // a ScannerTimeoutException. Else, it's because the region moved and we used the old - // id against the new region server; reset the scanner. - if (timeout < System.currentTimeMillis()) { - long elapsed = System.currentTimeMillis() - lastNext; - ScannerTimeoutException ex = - new ScannerTimeoutException(elapsed + "ms passed since the last invocation, " - + "timeout is currently set to " + scannerTimeout); - ex.initCause(e); - throw ex; - } - } else { - // If exception is any but the list below throw it back to the client; else setup - // the scanner and retry. - Throwable cause = e.getCause(); - if ((cause != null && cause instanceof NotServingRegionException) || - (cause != null && cause instanceof RegionServerStoppedException) || - e instanceof OutOfOrderScannerNextException) { - // 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. - } else { - throw e; - } - } - // Else, its signal from depths of ScannerCallable that we need to reset the scanner. - if (this.lastResult != null) { - // The region has moved. We need to open a brand new scanner at - // the new location. - // Reset the startRow to the row we've seen last so that the new - // scanner starts at the correct row. Otherwise we may see previously - // returned rows again. - // (ScannerCallable by now has "relocated" the correct region) - if (scan.isReversed()) { - scan.setStartRow(createClosestRowBefore(lastResult.getRow())); - } else { - scan.setStartRow(Bytes.add(lastResult.getRow(), new byte[1])); - } - } - if (e instanceof OutOfOrderScannerNextException) { - if (retryAfterOutOfOrderException) { - retryAfterOutOfOrderException = false; - } else { - // TODO: Why wrap this in a DNRIOE when it already is a DNRIOE? - throw new DoNotRetryIOException("Failed after retry of " + - "OutOfOrderScannerNextException: was there a rpc timeout?", e); - } - } - // Clear region. - this.currentRegion = null; - // Set this to zero so we don't try and do an rpc and close on remote server when - // the exception we got was UnknownScanner or the Server is going down. - callable = null; - - // This continue will take us to while at end of loop where we will set up new scanner. - continue; - } - long currentTime = System.currentTimeMillis(); - if (this.scanMetrics != null) { - this.scanMetrics.sumOfMillisSecBetweenNexts.addAndGet(currentTime - lastNext); - } - lastNext = currentTime; - // Groom the array of Results that we received back from the server before adding that - // Results to the scanner's cache. If partial results are not allowed to be seen by the - // caller, all book keeping will be performed within this method. - List resultsToAddToCache = getResultsToAddToCache(values); - if (!resultsToAddToCache.isEmpty()) { - for (Result rs : resultsToAddToCache) { - cache.add(rs); - // We don't make Iterator here - for (Cell cell : rs.rawCells()) { - remainingResultSize -= CellUtil.estimatedHeapSizeOf(cell); - } - countdown--; - this.lastResult = rs; - } - } - // Values == null means server-side filter has determined we must STOP - // !partialResults.isEmpty() means that we are still accumulating partial Results for a - // row. We should not change scanners before we receive all the partial Results for that - // row. - } while (remainingResultSize > 0 && countdown > 0 - && (!partialResults.isEmpty() || possiblyNextScanner(countdown, values == null))); + loadCache(); } if (cache.size() > 0) { @@ -491,6 +372,145 @@ public class ClientScanner extends AbstractClientScanner { } /** + * Contact the servers to load more {@link Result}s in the cache. + */ + protected void loadCache() throws IOException { + ScanResultWithContext values = null; + long remainingResultSize = maxScannerResultSize; + int countdown = this.caching; + + // We need to reset it if it's a new callable that was created + // with a countdown in nextScanner + callable.setCaching(this.caching); + // This flag is set when we want to skip the result returned. We do + // this when we reset scanner because it split under us. + boolean retryAfterOutOfOrderException = true; + // We don't expect that the server will have more results for us if + // it doesn't tell us otherwise. We rely on the size or count of results + boolean serverHasMoreResults = false; + do { + try { + // Server returns a null values if scanning is to stop. Else, + // returns an empty array if scanning is to go on and we've just + // exhausted current region. + values = call(callable, caller, scannerTimeout); + + // When the replica switch happens, we need to do certain operations + // again. The callable will openScanner with the right startkey + // but we need to pick up from there. Bypass the rest of the loop + // and let the catch-up happen in the beginning of the loop as it + // happens for the cases where we see exceptions. Since only openScanner + // would have happened, values would be null + if (values == null && callable.switchedToADifferentReplica()) { + this.currentRegion = callable.getHRegionInfo(); + continue; + } + retryAfterOutOfOrderException = true; + } catch (DoNotRetryIOException e) { + // 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. + if (e instanceof UnknownScannerException) { + long timeout = lastNext + scannerTimeout; + // If we are over the timeout, throw this exception to the client wrapped in + // a ScannerTimeoutException. Else, it's because the region moved and we used the old + // id against the new region server; reset the scanner. + if (timeout < System.currentTimeMillis()) { + long elapsed = System.currentTimeMillis() - lastNext; + ScannerTimeoutException ex = + new ScannerTimeoutException(elapsed + "ms passed since the last invocation, " + + "timeout is currently set to " + scannerTimeout); + ex.initCause(e); + throw ex; + } + } else { + // If exception is any but the list below throw it back to the client; else setup + // the scanner and retry. + Throwable cause = e.getCause(); + if ((cause != null && cause instanceof NotServingRegionException) || + (cause != null && cause instanceof RegionServerStoppedException) || + e instanceof OutOfOrderScannerNextException) { + // 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. + } else { + throw e; + } + } + // Else, its signal from depths of ScannerCallable that we need to reset the scanner. + if (this.lastResult != null) { + // The region has moved. We need to open a brand new scanner at + // the new location. + // Reset the startRow to the row we've seen last so that the new + // scanner starts at the correct row. Otherwise we may see previously + // returned rows again. + // (ScannerCallable by now has "relocated" the correct region) + if (scan.isReversed()) { + scan.setStartRow(createClosestRowBefore(lastResult.getRow())); + } else { + scan.setStartRow(Bytes.add(lastResult.getRow(), new byte[1])); + } + } + if (e instanceof OutOfOrderScannerNextException) { + if (retryAfterOutOfOrderException) { + retryAfterOutOfOrderException = false; + } else { + // TODO: Why wrap this in a DNRIOE when it already is a DNRIOE? + throw new DoNotRetryIOException("Failed after retry of " + + "OutOfOrderScannerNextException: was there a rpc timeout?", e); + } + } + // Clear region. + this.currentRegion = null; + // Set this to zero so we don't try and do an rpc and close on remote server when + // the exception we got was UnknownScanner or the Server is going down. + callable = null; + + // This continue will take us to while at end of loop where we will set up new scanner. + continue; + } + long currentTime = System.currentTimeMillis(); + if (this.scanMetrics != null) { + this.scanMetrics.sumOfMillisSecBetweenNexts.addAndGet(currentTime - lastNext); + } + lastNext = currentTime; + // Groom the array of Results that we received back from the server before adding that + // Results to the scanner's cache. If partial results are not allowed to be seen by the + // caller, all book keeping will be performed within this method. + Result[] serverResults = (null != values ? values.getResults() : null); + List resultsToAddToCache = getResultsToAddToCache(serverResults); + if (!resultsToAddToCache.isEmpty()) { + for (Result rs : resultsToAddToCache) { + cache.add(rs); + // We don't make Iterator here + for (Cell cell : rs.rawCells()) { + remainingResultSize -= CellUtil.estimatedHeapSizeOf(cell); + } + countdown--; + this.lastResult = rs; + } + } + // We expect that the server won't have more results for us when we exhaust + // the size (bytes or count) of the results returned. If the server *does* inform us that + // there are more results, we want to avoid possiblyNextScanner(...). Only when we actually + // get results is the moreResults context valid. + if (null != values && values.getResults().length > 0 && values.getHasMoreResultsContext()) { + // Only adhere to more server results when we don't have any partialResults + // as it keeps the outer loop logic the same. + serverHasMoreResults = values.getServerHasMoreResults() & partialResults.isEmpty(); + } + // Values == null means server-side filter has determined we must STOP + // !partialResults.isEmpty() means that we are still accumulating partial Results for a + // row. We should not change scanners before we receive all the partial Results for that + // row. + } while (remainingResultSize > 0 && countdown > 0 && !serverHasMoreResults + && (!partialResults.isEmpty() || possiblyNextScanner(countdown, values == null))); + } + + /** * This method ensures all of our book keeping regarding partial results is kept up to date. This * method should be called once we know that the results we received back from the RPC request do * not contain errors. We return a list of results that should be added to the cache. In general, diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallReversedScanner.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallReversedScanner.java index 2cab830..19fd4ce 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallReversedScanner.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallReversedScanner.java @@ -142,7 +142,10 @@ public class ClientSmallReversedScanner extends ReversedClientScanner { // exhausted current region. // callWithoutRetries is at this layer. Within the ScannerCallableWithReplicas, // we do a callWithRetries - values = this.caller.callWithoutRetries(smallScanCallable, scannerTimeout); + // TODO use context from server + ScanResultWithContext resultsWithContext = this.caller.callWithoutRetries( + smallScanCallable, scannerTimeout); + values = (null != resultsWithContext ? resultsWithContext.getResults() : null); this.currentRegion = smallScanCallable.getHRegionInfo(); long currentTime = System.currentTimeMillis(); if (this.scanMetrics != null) { diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallScanner.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallScanner.java index 5b9f627..0df5bba 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallScanner.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallScanner.java @@ -136,11 +136,11 @@ public class ClientSmallScanner extends ClientScanner { } - static ScannerCallableWithReplicas getSmallScanCallable( - ClusterConnection connection, TableName table, Scan scan, - ScanMetrics scanMetrics, byte[] localStartKey, final int cacheNum, - RpcControllerFactory controllerFactory, ExecutorService pool, int primaryOperationTimeout, - int retries, int scannerTimeout, Configuration conf, RpcRetryingCaller caller) { + static ScannerCallableWithReplicas getSmallScanCallable(ClusterConnection connection, + TableName table, Scan scan, ScanMetrics scanMetrics, byte[] localStartKey, + final int cacheNum, RpcControllerFactory controllerFactory, ExecutorService pool, + int primaryOperationTimeout, int retries, int scannerTimeout, Configuration conf, + RpcRetryingCaller caller) { scan.setStartRow(localStartKey); SmallScannerCallable s = new SmallScannerCallable( connection, table, scan, scanMetrics, controllerFactory, cacheNum, 0); @@ -160,7 +160,7 @@ public class ClientSmallScanner extends ClientScanner { } @Override - public Result[] call(int timeout) throws IOException { + public ScanResultWithContext call(int timeout) throws IOException { if (this.closed) return null; if (Thread.interrupted()) { throw new InterruptedIOException(); @@ -173,8 +173,13 @@ public class ClientSmallScanner extends ClientScanner { controller.setPriority(getTableName()); controller.setCallTimeout(timeout); response = getStub().scan(controller, request); - return ResponseConverter.getResults(controller.cellScanner(), + Result[] results = ResponseConverter.getResults(controller.cellScanner(), response); + if (response.hasMoreResults()) { + return new ScanResultWithContext(results, response.getMoreResults()); + } else { + return new ScanResultWithContext(results); + } } catch (ServiceException se) { throw ProtobufUtil.getRemoteException(se); } @@ -207,7 +212,10 @@ public class ClientSmallScanner extends ClientScanner { // exhausted current region. // callWithoutRetries is at this layer. Within the ScannerCallableWithReplicas, // we do a callWithRetries - values = this.caller.callWithoutRetries(smallScanCallable, scannerTimeout); + // TODO Use the server's response about more results + ScanResultWithContext resultsWithContext = this.caller.callWithoutRetries( + smallScanCallable, scannerTimeout); + values = (null != resultsWithContext ? resultsWithContext.getResults() : null); this.currentRegion = smallScanCallable.getHRegionInfo(); long currentTime = System.currentTimeMillis(); if (this.scanMetrics != null) { diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScanResultWithContext.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScanResultWithContext.java new file mode 100644 index 0000000..14cf52f --- /dev/null +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScanResultWithContext.java @@ -0,0 +1,133 @@ +/** + * 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.client; + +import java.util.Arrays; + +import org.apache.commons.lang.builder.HashCodeBuilder; +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.classification.InterfaceAudience; + +/** + * Encapsulates the results from a Scan, optionally with additionally + * information from the RegionServer. + */ +@InterfaceAudience.Private +public class ScanResultWithContext { + + /** + * Results from the server + */ + private final Result[] results; + /** + * Was more information on the presence of more results + * on the server returned? + */ + private final boolean hasMoreResultsContext; + /** + * Do more results exist on the server. Only valid if + * {@link #hasAddtlResultsContext} is true. + */ + private final Boolean hasMoreResults; + + public ScanResultWithContext(Result[] results) { + this.results = results; + this.hasMoreResultsContext = false; + this.hasMoreResults = null; + } + + public ScanResultWithContext(Result[] results, boolean hasMoreResults) { + this.results = results; + this.hasMoreResultsContext = true; + this.hasMoreResults = hasMoreResults; + } + + public Result[] getResults() { + return results; + } + + public boolean getHasMoreResultsContext() { + return hasMoreResultsContext; + } + + public boolean getServerHasMoreResults() { + if (!hasMoreResultsContext) { + throw new IllegalStateException("Context doesn't contain server response whether " + + "the server contains more results"); + } + assert null != hasMoreResults; + return hasMoreResults; + } + + @Override + public String toString() { + StringBuilder sb = new StringBuilder(32); + sb.append("ScanResultWithContext[results=").append(Arrays.toString(results)); + sb.append(", hasMoreResultsContext=").append(hasMoreResultsContext); + if (hasMoreResultsContext) { + sb.append(", hasMoreResults=").append(hasMoreResults); + } + return sb.toString(); + } + + /* Result doesn't have hashCode and equals implemented, so it's pointless + * to try to implement these here. + * + @Override + public int hashCode() { + HashCodeBuilder hcb = new HashCodeBuilder(17,31); + hcb.append(results).append(hasMoreResultsContext); + if (hasMoreResultsContext) { + hcb.append(hasMoreResults); + } + return hcb.toHashCode(); + } + + @Override + public boolean equals(Object o) { + if (o instanceof ScanResultWithContext) { + ScanResultWithContext other = (ScanResultWithContext) o; + + if (!Arrays.equals(results, other.results)) { + return false; + } + + if (hasMoreResultsContext != other.hasMoreResultsContext) { + return false; + } + + if (null == hasMoreResults) { + // One is null, the other isn't + if (null != other.hasMoreResults) { + return false; + } + // Both are null + return true; + } else { + // One is null, the other isn't + if (null == other.hasMoreResults) { + return false; + } + + // Check boolean equality + return hasMoreResults.equals(other.hasMoreResults); + } + } + return false; + }*/ +} diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java index 2fb5966..3ecb1f3 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java @@ -60,7 +60,7 @@ import com.google.protobuf.TextFormat; * {@link RpcRetryingCaller} so fails are retried. */ @InterfaceAudience.Private -public class ScannerCallable extends RegionServerCallable { +public class ScannerCallable extends RegionServerCallable { public static final String LOG_SCANNER_LATENCY_CUTOFF = "hbase.client.log.scanner.latency.cutoff"; public static final String LOG_SCANNER_ACTIVITY = "hbase.client.log.scanner.activity"; @@ -179,7 +179,7 @@ public class ScannerCallable extends RegionServerCallable { @Override @SuppressWarnings("deprecation") - public Result [] call(int callTimeout) throws IOException { + public ScanResultWithContext call(int callTimeout) throws IOException { if (Thread.interrupted()) { throw new InterruptedIOException(); } @@ -193,6 +193,7 @@ public class ScannerCallable extends RegionServerCallable { } else { Result [] rrs = null; ScanRequest request = null; + ScanResultWithContext resultWithContext; try { incRPCcallsMetrics(); request = RequestConverter.buildScanRequest(scannerId, caching, false, nextCallSeq); @@ -224,11 +225,22 @@ public class ScannerCallable extends RegionServerCallable { + rows + " rows from scanner=" + scannerId); } } - if (response.hasMoreResults() - && !response.getMoreResults()) { - scannerId = -1L; - closed = true; - return null; + if (response.hasMoreResults()) { + if (!response.getMoreResults()) { + scannerId = -1L; + closed = true; + if (null == rrs) { + return null; + } + // No more results + resultWithContext = new ScanResultWithContext(rrs, false); + } else { + // The server has more results + resultWithContext = new ScanResultWithContext(rrs, true); + } + } else { + // Server didn't respond whether it has more results or not. + resultWithContext = new ScanResultWithContext(rrs); } } catch (ServiceException se) { throw ProtobufUtil.getRemoteException(se); @@ -276,7 +288,7 @@ public class ScannerCallable extends RegionServerCallable { throw ioe; } } - return rrs; + return resultWithContext; } } return null; diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallableWithReplicas.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallableWithReplicas.java index 38a6481..ff28350 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallableWithReplicas.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallableWithReplicas.java @@ -59,7 +59,7 @@ import com.google.common.annotations.VisibleForTesting; * */ @InterfaceAudience.Private -class ScannerCallableWithReplicas implements RetryingCallable { +class ScannerCallableWithReplicas implements RetryingCallable { private final Log LOG = LogFactory.getLog(this.getClass()); volatile ScannerCallable currentScannerCallable; AtomicBoolean replicaSwitched = new AtomicBoolean(false); @@ -69,7 +69,7 @@ class ScannerCallableWithReplicas implements RetryingCallable { private final Scan scan; private final int retries; private Result lastResult; - private final RpcRetryingCaller caller; + private final RpcRetryingCaller caller; private final TableName tableName; private Configuration conf; private int scannerTimeout; @@ -79,7 +79,7 @@ class ScannerCallableWithReplicas implements RetryingCallable { public ScannerCallableWithReplicas(TableName tableName, ClusterConnection cConnection, ScannerCallable baseCallable, ExecutorService pool, int timeBeforeReplicas, Scan scan, int retries, int scannerTimeout, int caching, Configuration conf, - RpcRetryingCaller caller) { + RpcRetryingCaller caller) { this.currentScannerCallable = baseCallable; this.cConnection = cConnection; this.pool = pool; @@ -112,7 +112,7 @@ class ScannerCallableWithReplicas implements RetryingCallable { } @Override - public Result [] call(int timeout) throws IOException { + public ScanResultWithContext call(int timeout) throws IOException { // If the active replica callable was closed somewhere, invoke the RPC to // really close it. In the case of regular scanners, this applies. We make couple // of RPCs to a RegionServer, and when that region is exhausted, we set @@ -123,7 +123,7 @@ class ScannerCallableWithReplicas implements RetryingCallable { if (LOG.isTraceEnabled()) { LOG.trace("Closing scanner id=" + currentScannerCallable.scannerId); } - Result[] r = currentScannerCallable.call(timeout); + ScanResultWithContext r = currentScannerCallable.call(timeout); currentScannerCallable = null; return r; } @@ -140,8 +140,8 @@ class ScannerCallableWithReplicas implements RetryingCallable { // allocate a boundedcompletion pool of some multiple of number of replicas. // We want to accomodate some RPCs for redundant replica scans (but are still in progress) - ResultBoundedCompletionService> cs = - new ResultBoundedCompletionService>( + ResultBoundedCompletionService> cs = + new ResultBoundedCompletionService>( new RpcRetryingCallerFactory(ScannerCallableWithReplicas.this.conf), pool, rl.size() * 5); @@ -153,10 +153,10 @@ class ScannerCallableWithReplicas implements RetryingCallable { submitted += addCallsForCurrentReplica(cs, rl); try { // wait for the timeout to see whether the primary responds back - Future> f = cs.poll(timeBeforeReplicas, + Future> f = cs.poll(timeBeforeReplicas, TimeUnit.MICROSECONDS); // Yes, microseconds if (f != null) { - Pair r = f.get(); + Pair r = f.get(); if (r != null && r.getSecond() != null) { updateCurrentlyServingReplica(r.getSecond(), r.getFirst(), done, pool); } @@ -179,8 +179,8 @@ class ScannerCallableWithReplicas implements RetryingCallable { try { while (completed < submitted) { try { - Future> f = cs.take(); - Pair r = f.get(); + Future> f = cs.take(); + Pair r = f.get(); if (r != null && r.getSecond() != null) { updateCurrentlyServingReplica(r.getSecond(), r.getFirst(), done, pool); } @@ -210,8 +210,9 @@ class ScannerCallableWithReplicas implements RetryingCallable { return null; // unreachable } - private void updateCurrentlyServingReplica(ScannerCallable scanner, Result[] result, - AtomicBoolean done, ExecutorService pool) { + private void updateCurrentlyServingReplica(ScannerCallable scanner, + ScanResultWithContext resultWithContext, AtomicBoolean done, ExecutorService pool) { + Result[] result = (null != resultWithContext ? resultWithContext.getResults() : null); if (done.compareAndSet(false, true)) { if (currentScannerCallable != scanner) replicaSwitched.set(true); currentScannerCallable = scanner; @@ -258,7 +259,8 @@ class ScannerCallableWithReplicas implements RetryingCallable { } private int addCallsForCurrentReplica( - ResultBoundedCompletionService> cs, RegionLocations rl) { + ResultBoundedCompletionService> cs, + RegionLocations rl) { RetryingRPC retryingOnReplica = new RetryingRPC(currentScannerCallable); outstandingCallables.add(currentScannerCallable); cs.submit(retryingOnReplica, scannerTimeout, currentScannerCallable.id); @@ -266,8 +268,8 @@ class ScannerCallableWithReplicas implements RetryingCallable { } private int addCallsForOtherReplicas( - ResultBoundedCompletionService> cs, RegionLocations rl, - int min, int max) { + ResultBoundedCompletionService> cs, + RegionLocations rl, int min, int max) { if (scan.getConsistency() == Consistency.STRONG) { return 0; // not scheduling on other replicas for strong consistency } @@ -296,9 +298,10 @@ class ScannerCallableWithReplicas implements RetryingCallable { return someRPCcancelled; } - class RetryingRPC implements RetryingCallable>, Cancellable { + class RetryingRPC implements RetryingCallable>, + Cancellable { final ScannerCallable callable; - RpcRetryingCaller caller; + RpcRetryingCaller caller; private volatile boolean cancelled = false; RetryingRPC(ScannerCallable callable) { @@ -311,19 +314,19 @@ class ScannerCallableWithReplicas implements RetryingCallable { this.caller = ScannerCallableWithReplicas.this.caller; if (scan.getConsistency() == Consistency.TIMELINE) { this.caller = new RpcRetryingCallerFactory(ScannerCallableWithReplicas.this.conf). - newCaller(); + newCaller(); } } @Override - public Pair call(int callTimeout) throws IOException { + public Pair call(int callTimeout) throws IOException { // since the retries is done within the ResultBoundedCompletionService, // we don't invoke callWithRetries here if (cancelled) { return null; } - Result[] res = this.caller.callWithoutRetries(this.callable, callTimeout); - return new Pair(res, this.callable); + ScanResultWithContext results = this.caller.callWithoutRetries(this.callable, callTimeout); + return new Pair(results, this.callable); } @Override diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestClientScanner.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestClientScanner.java new file mode 100644 index 0000000..42d8721 --- /dev/null +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestClientScanner.java @@ -0,0 +1,460 @@ +/** + * 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.client; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertFalse; + +import java.io.IOException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.CellScanner; +import org.apache.hadoop.hbase.KeyValue; +import org.apache.hadoop.hbase.KeyValue.Type; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.ipc.RpcControllerFactory; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.mockito.InOrder; +import org.mockito.Mockito; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; + +/** + * Test the ClientScanner. + */ +@Category(SmallTests.class) +public class TestClientScanner { + + Scan scan; + ExecutorService pool; + Configuration conf; + + ClusterConnection clusterConn; + RpcRetryingCallerFactory rpcFactory; + RpcControllerFactory controllerFactory; + + @Before + @SuppressWarnings("deprecation") + public void setup() throws IOException { + clusterConn = Mockito.mock(ClusterConnection.class); + rpcFactory = Mockito.mock(RpcRetryingCallerFactory.class); + controllerFactory = Mockito.mock(RpcControllerFactory.class); + pool = Executors.newSingleThreadExecutor(); + scan = new Scan(); + conf = new Configuration(); + Mockito.when(clusterConn.getConfiguration()).thenReturn(conf); + } + + @After + public void teardown() { + if (null != pool) { + pool.shutdownNow(); + } + } + + private static class MockClientScanner extends ClientScanner { + + private boolean rpcFinished = false; + private boolean rpcFinishedFired = false; + + public MockClientScanner(final Configuration conf, final Scan scan, final TableName tableName, + ClusterConnection connection, RpcRetryingCallerFactory rpcFactory, + RpcControllerFactory controllerFactory, ExecutorService pool, int primaryOperationTimeout) + throws IOException { + super(conf, scan, tableName, connection, rpcFactory, controllerFactory, pool, + primaryOperationTimeout); + } + + @Override + protected boolean nextScanner(int nbRows, final boolean done) throws IOException { + if (!rpcFinished) { + return super.nextScanner(nbRows, done); + } + + // Enforce that we don't short-circuit more than once + if (rpcFinishedFired) { + throw new RuntimeException("Expected nextScanner to only be called once after " + + " short-circuit was triggered."); + } + rpcFinishedFired = true; + return false; + } + + public void setRpcFinished(boolean rpcFinished) { + this.rpcFinished = rpcFinished; + } + } + + @Test + @SuppressWarnings("unchecked") + public void testNoResultsHint() throws IOException { + Result[] results = new Result[1]; + KeyValue kv1 = new KeyValue("row".getBytes(), "cf".getBytes(), "cq".getBytes(), 1, + Type.Maximum); + results[0] = Result.create(new Cell[] {kv1}); + final ScanResultWithContext resultWithContext1 = new ScanResultWithContext(results); + + RpcRetryingCaller caller = Mockito.mock(RpcRetryingCaller.class); + + Mockito.when(rpcFactory. newCaller()).thenReturn(caller); + Mockito.when(caller.callWithoutRetries(Mockito.any(RetryingCallable.class), + Mockito.anyInt())).thenAnswer(new Answer() { + private int count = 0; + @Override + public ScanResultWithContext answer(InvocationOnMock invocation) throws Throwable { + switch (count) { + case 0: // initialize + case 2: // close + count++; + return null; + case 1: + count++; + return resultWithContext1; + default: + throw new RuntimeException("Expected only 2 invocations"); + } + } + }); + + // Set a much larger cache and buffer size than we'll provide + scan.setCaching(100); + scan.setMaxResultSize(1000*1000); + + try (MockClientScanner scanner = new MockClientScanner(conf, scan, TableName.valueOf("table"), + clusterConn, rpcFactory, controllerFactory, pool, Integer.MAX_VALUE)) { + + scanner.setRpcFinished(true); + + InOrder inOrder = Mockito.inOrder(caller); + + scanner.loadCache(); + + // One more call due to initializeScannerInConstruction() + inOrder.verify(caller, Mockito.times(2)).callWithoutRetries( + Mockito.any(RetryingCallable.class), Mockito.anyInt()); + + assertEquals(1, scanner.cache.size()); + Result r = scanner.cache.poll(); + assertNotNull(r); + CellScanner cs = r.cellScanner(); + assertTrue(cs.advance()); + assertEquals(kv1, cs.current()); + assertFalse(cs.advance()); + } + } + + @Test + @SuppressWarnings("unchecked") + public void testSizeLimit() throws IOException { + Result[] results = new Result[1]; + KeyValue kv1 = new KeyValue("row".getBytes(), "cf".getBytes(), "cq".getBytes(), 1, + Type.Maximum); + results[0] = Result.create(new Cell[] {kv1}); + final ScanResultWithContext resultWithContext1 = new ScanResultWithContext(results, false); + + RpcRetryingCaller caller = Mockito.mock(RpcRetryingCaller.class); + + Mockito.when(rpcFactory. newCaller()).thenReturn(caller); + Mockito.when(caller.callWithoutRetries(Mockito.any(RetryingCallable.class), + Mockito.anyInt())).thenAnswer(new Answer() { + private int count = 0; + @Override + public ScanResultWithContext answer(InvocationOnMock invocation) throws Throwable { + switch (count) { + case 0: // initialize + case 2: // close + count++; + return null; + case 1: + count++; + return resultWithContext1; + default: + throw new RuntimeException("Expected only 2 invocations"); + } + } + }); + + Mockito.when(rpcFactory. newCaller()).thenReturn(caller); + + // Set a much larger cache + scan.setCaching(100); + // The single key-value will exit the loop + scan.setMaxResultSize(1); + + try (MockClientScanner scanner = new MockClientScanner(conf, scan, TableName.valueOf("table"), + clusterConn, rpcFactory, controllerFactory, pool, Integer.MAX_VALUE)) { + + // Due to initializeScannerInConstruction() + Mockito.verify(caller).callWithoutRetries(Mockito.any(RetryingCallable.class), + Mockito.anyInt()); + + InOrder inOrder = Mockito.inOrder(caller); + + scanner.loadCache(); + + inOrder.verify(caller, Mockito.times(2)).callWithoutRetries( + Mockito.any(RetryingCallable.class), Mockito.anyInt()); + + assertEquals(1, scanner.cache.size()); + Result r = scanner.cache.poll(); + assertNotNull(r); + CellScanner cs = r.cellScanner(); + assertTrue(cs.advance()); + assertEquals(kv1, cs.current()); + assertFalse(cs.advance()); + } + } + + @Test + @SuppressWarnings("unchecked") + public void testCacheLimit() throws IOException { + KeyValue kv1 = new KeyValue("row1".getBytes(), "cf".getBytes(), "cq".getBytes(), 1, + Type.Maximum), kv2 = new KeyValue("row2".getBytes(), "cf".getBytes(), "cq".getBytes(), 1, + Type.Maximum), kv3 = new KeyValue("row3".getBytes(), "cf".getBytes(), "cq".getBytes(), 1, + Type.Maximum); + Result[] results = new Result[] {Result.create(new Cell[] {kv1}), + Result.create(new Cell[] {kv2}), Result.create(new Cell[] {kv3})}; + final ScanResultWithContext resultWithContext1 = new ScanResultWithContext(results, false); + + RpcRetryingCaller caller = Mockito.mock(RpcRetryingCaller.class); + + Mockito.when(rpcFactory. newCaller()).thenReturn(caller); + Mockito.when(caller.callWithoutRetries(Mockito.any(RetryingCallable.class), + Mockito.anyInt())).thenAnswer(new Answer() { + private int count = 0; + @Override + public ScanResultWithContext answer(InvocationOnMock invocation) throws Throwable { + switch (count) { + case 0: // initialize + case 2: // close + count++; + return null; + case 1: + count++; + return resultWithContext1; + default: + throw new RuntimeException("Expected only 2 invocations"); + } + } + }); + + Mockito.when(rpcFactory. newCaller()).thenReturn(caller); + + // Set a small cache + scan.setCaching(1); + // Set a very large size + scan.setMaxResultSize(1000*1000); + + try (MockClientScanner scanner = new MockClientScanner(conf, scan, TableName.valueOf("table"), + clusterConn, rpcFactory, controllerFactory, pool, Integer.MAX_VALUE)) { + + // Due to initializeScannerInConstruction() + Mockito.verify(caller).callWithoutRetries(Mockito.any(RetryingCallable.class), + Mockito.anyInt()); + + InOrder inOrder = Mockito.inOrder(caller); + + scanner.loadCache(); + + // Ensures that possiblyNextScanner isn't called at the end which would trigger + // another call to callWithoutRetries + inOrder.verify(caller, Mockito.times(2)).callWithoutRetries( + Mockito.any(RetryingCallable.class), Mockito.anyInt()); + + assertEquals(3, scanner.cache.size()); + Result r = scanner.cache.poll(); + assertNotNull(r); + CellScanner cs = r.cellScanner(); + assertTrue(cs.advance()); + assertEquals(kv1, cs.current()); + assertFalse(cs.advance()); + + r = scanner.cache.poll(); + assertNotNull(r); + cs = r.cellScanner(); + assertTrue(cs.advance()); + assertEquals(kv2, cs.current()); + assertFalse(cs.advance()); + + r = scanner.cache.poll(); + assertNotNull(r); + cs = r.cellScanner(); + assertTrue(cs.advance()); + assertEquals(kv3, cs.current()); + assertFalse(cs.advance()); + } + } + + @Test + @SuppressWarnings("unchecked") + public void testNoMoreResults() throws IOException { + Result[] results = new Result[1]; + KeyValue kv1 = new KeyValue("row".getBytes(), "cf".getBytes(), "cq".getBytes(), 1, + Type.Maximum); + results[0] = Result.create(new Cell[] {kv1}); + final ScanResultWithContext resultWithContext1 = new ScanResultWithContext(results, false); + + RpcRetryingCaller caller = Mockito.mock(RpcRetryingCaller.class); + + Mockito.when(rpcFactory. newCaller()).thenReturn(caller); + Mockito.when(caller.callWithoutRetries(Mockito.any(RetryingCallable.class), + Mockito.anyInt())).thenAnswer(new Answer() { + private int count = 0; + @Override + public ScanResultWithContext answer(InvocationOnMock invocation) throws Throwable { + switch (count) { + case 0: // initialize + case 2: // close + count++; + return null; + case 1: + count++; + return resultWithContext1; + default: + throw new RuntimeException("Expected only 2 invocations"); + } + } + }); + + Mockito.when(rpcFactory. newCaller()).thenReturn(caller); + + // Set a much larger cache and buffer size than we'll provide + scan.setCaching(100); + scan.setMaxResultSize(1000*1000); + + try (MockClientScanner scanner = new MockClientScanner(conf, scan, TableName.valueOf("table"), + clusterConn, rpcFactory, controllerFactory, pool, Integer.MAX_VALUE)) { + + // Due to initializeScannerInConstruction() + Mockito.verify(caller).callWithoutRetries(Mockito.any(RetryingCallable.class), + Mockito.anyInt()); + + scanner.setRpcFinished(true); + + InOrder inOrder = Mockito.inOrder(caller); + + scanner.loadCache(); + + inOrder.verify(caller, Mockito.times(2)).callWithoutRetries( + Mockito.any(RetryingCallable.class), Mockito.anyInt()); + + assertEquals(1, scanner.cache.size()); + Result r = scanner.cache.poll(); + assertNotNull(r); + CellScanner cs = r.cellScanner(); + assertTrue(cs.advance()); + assertEquals(kv1, cs.current()); + assertFalse(cs.advance()); + } + } + + @Test + @SuppressWarnings("unchecked") + public void testMoreResults() throws IOException { + Result[] results = new Result[1]; + KeyValue kv1 = new KeyValue("row".getBytes(), "cf".getBytes(), "cq".getBytes(), 1, + Type.Maximum); + results[0] = Result.create(new Cell[] {kv1}); + final ScanResultWithContext resultWithContext1 = new ScanResultWithContext(results, true); + + results = new Result[1]; + KeyValue kv2 = new KeyValue("row2".getBytes(), "cf".getBytes(), "cq".getBytes(), 1, + Type.Maximum); + results[0] = Result.create(new Cell[] {kv2}); + + // The server reports back nothing WRT more results + final ScanResultWithContext resultWithContext2 = new ScanResultWithContext(results, false); + + RpcRetryingCaller caller = Mockito.mock(RpcRetryingCaller.class); + + Mockito.when(rpcFactory. newCaller()).thenReturn(caller); + Mockito.when(caller.callWithoutRetries(Mockito.any(RetryingCallable.class), + Mockito.anyInt())).thenAnswer(new Answer() { + private int count = 0; + @Override + public ScanResultWithContext answer(InvocationOnMock invocation) throws Throwable { + switch (count) { + case 0: // initialize + case 3: // close + count++; + return null; + case 1: + count++; + return resultWithContext1; + case 2: + count++; + return resultWithContext2; + default: + throw new RuntimeException("Expected only 2 invocations"); + } + } + }); + + // Set a much larger cache and buffer size than we'll provide + scan.setCaching(100); + scan.setMaxResultSize(1000*1000); + + try (MockClientScanner scanner = new MockClientScanner(conf, scan, TableName.valueOf("table"), + clusterConn, rpcFactory, controllerFactory, pool, Integer.MAX_VALUE)) { + + // Due to initializeScannerInConstruction() + Mockito.verify(caller).callWithoutRetries(Mockito.any(RetryingCallable.class), + Mockito.anyInt()); + + InOrder inOrder = Mockito.inOrder(caller); + + scanner.loadCache(); + + inOrder.verify(caller, Mockito.times(2)).callWithoutRetries( + Mockito.any(RetryingCallable.class), Mockito.anyInt()); + + assertEquals(1, scanner.cache.size()); + Result r = scanner.cache.poll(); + assertNotNull(r); + CellScanner cs = r.cellScanner(); + assertTrue(cs.advance()); + assertEquals(kv1, cs.current()); + assertFalse(cs.advance()); + + scanner.setRpcFinished(true); + + inOrder = Mockito.inOrder(caller); + + scanner.loadCache(); + + inOrder.verify(caller, Mockito.times(3)).callWithoutRetries( + Mockito.any(RetryingCallable.class), Mockito.anyInt()); + + r = scanner.cache.poll(); + assertNotNull(r); + cs = r.cellScanner(); + assertTrue(cs.advance()); + assertEquals(kv2, cs.current()); + assertFalse(cs.advance()); + } + } +} diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScanResultWithContext.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScanResultWithContext.java new file mode 100644 index 0000000..acac009 --- /dev/null +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScanResultWithContext.java @@ -0,0 +1,55 @@ +/** + * 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.client; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.KeyValue; +import org.apache.hadoop.hbase.KeyValue.Type; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +/** + * Tests for ScanResultWithContext + */ +@Category(SmallTests.class) +public class TestScanResultWithContext { + + @Test + public void testGetters() { + KeyValue kv1 = new KeyValue("row1".getBytes(), "cf".getBytes(), "cq".getBytes(), 1, Type.Maximum); + Result[] results = new Result[] {Result.create(new Cell[] {kv1})}; + + ScanResultWithContext ctx1 = new ScanResultWithContext(results); + + assertTrue(results == ctx1.getResults()); + + ScanResultWithContext ctx2 = new ScanResultWithContext(results, false); + ScanResultWithContext ctx3 = new ScanResultWithContext(results, true); + + assertTrue(ctx2.getHasMoreResultsContext()); + assertFalse(ctx2.getServerHasMoreResults()); + + assertTrue(ctx3.getHasMoreResultsContext()); + assertTrue(ctx3.getServerHasMoreResults()); + } + +} 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 ba7b70c..a85f8d1 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 @@ -2131,8 +2131,10 @@ public class RSRpcServices implements HBaseRPCErrorHandler, } if (!done) { + // Ultimately the maxResultSize from the client (or -1 if unset) long maxResultSize = scanner.getMaxResultSize(); if (maxResultSize <= 0) { + // Client provided no limit, use the server's limit maxResultSize = maxScannerResultSize; } List values = new ArrayList(); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSizeFailures.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSizeFailures.java new file mode 100644 index 0000000..7980c12 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSizeFailures.java @@ -0,0 +1,166 @@ +/** + * + * 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.client; + +import static org.junit.Assert.assertEquals; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedList; +import java.util.List; +import java.util.TreeSet; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.HColumnDescriptor; +import org.apache.hadoop.hbase.HTableDescriptor; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.testclassification.LargeTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category(LargeTests.class) +public class TestSizeFailures { + final Log LOG = LogFactory.getLog(getClass()); + protected final static HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility(); + private static byte [] FAMILY = Bytes.toBytes("testFamily"); + protected static int SLAVES = 1; + + /** + * @throws java.lang.Exception + */ + @BeforeClass + public static void setUpBeforeClass() throws Exception { + // Uncomment the following lines if more verbosity is needed for + // debugging (see HBASE-12285 for details). + //((Log4JLogger)RpcServer.LOG).getLogger().setLevel(Level.ALL); + //((Log4JLogger)RpcClient.LOG).getLogger().setLevel(Level.ALL); + //((Log4JLogger)ScannerCallable.LOG).getLogger().setLevel(Level.ALL); + Configuration conf = TEST_UTIL.getConfiguration(); + conf.setBoolean("hbase.table.sanity.checks", true); // ignore sanity checks in the server + // Disable compactions? +// conf.setInt("hbase.hstore.compaction.max", 100); +// conf.setInt("hbase.hstore.compaction.min", 100); + // We need more than one region server in this test + TEST_UTIL.startMiniCluster(SLAVES); + } + + /** + * @throws java.lang.Exception + */ + @AfterClass + public static void tearDownAfterClass() throws Exception { + TEST_UTIL.shutdownMiniCluster(); + } + + /** + * Basic client side validation of HBASE-13262 + */ + @Test + public void testScannerSeesAllRecords() throws Exception { + final int NUM_ROWS = 1000 * 1000, NUM_COLS = 10; + final TableName TABLENAME = TableName.valueOf("testScannerSeesAllRecords"); + List qualifiers = new ArrayList<>(); + for (int i = 1; i <= 10; i++) { + qualifiers.add(Bytes.toBytes(Integer.toString(i))); + } + + HColumnDescriptor hcd = new HColumnDescriptor(FAMILY); + HTableDescriptor desc = new HTableDescriptor(TABLENAME); + desc.addFamily(hcd); + byte[][] splits = new byte[9][2]; + for (int i = 1; i < 10; i++) { + int split = 48 + i; + splits[i - 1][0] = (byte) (split >>> 8); + splits[i - 1][0] = (byte) (split); + } + TEST_UTIL.getHBaseAdmin().createTable(desc, splits); + Connection conn = TEST_UTIL.getConnection(); + + try (Table table = conn.getTable(TABLENAME)) { + List puts = new LinkedList<>(); + for (int i = 0; i < NUM_ROWS; i++) { + Put p = new Put(Bytes.toBytes(Integer.toString(i))); + for (int j = 0; j < NUM_COLS; j++) { + byte[] value = new byte[50]; + Bytes.random(value); + p.addColumn(FAMILY, Bytes.toBytes(Integer.toString(j)), value); + } + puts.add(p); + + if (puts.size() == 1000) { + Object[] results = new Object[1000]; + try { + table.batch(puts, results); + } catch (IOException e) { + LOG.error("Failed to write data", e); + LOG.debug("Errors: " + Arrays.toString(results)); + } + + puts.clear(); + } + } + + if (puts.size() > 0) { + Object[] results = new Object[puts.size()]; + try { + table.batch(puts, results); + } catch (IOException e) { + LOG.error("Failed to write data", e); + LOG.debug("Errors: " + Arrays.toString(results)); + } + } + + // Flush the memstore to disk + TEST_UTIL.getHBaseAdmin().flush(TABLENAME); + + TreeSet rows = new TreeSet<>(); + long rowsObserved = 0l; + long entriesObserved = 0l; + Scan s = new Scan(); + s.addFamily(FAMILY); + s.setMaxResultSize(-1); + s.setBatch(-1); + s.setCaching(500); + ResultScanner scanner = table.getScanner(s); + // Read all the records in the table + for (Result result : scanner) { + rowsObserved++; + String row = new String(result.getRow()); + rows.add(Integer.parseInt(row)); + while (result.advance()) { + entriesObserved++; + // result.current(); + } + } + + // Verify that we see 1M rows and 10M cells + assertEquals(NUM_ROWS, rowsObserved); + assertEquals(NUM_ROWS * NUM_COLS, entriesObserved); + } + + conn.close(); + } +} diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java index 9e8097e..f37b324 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java @@ -637,7 +637,7 @@ public class TestAssignmentManager { } final ScanResponse.Builder builder = ScanResponse.newBuilder(); - builder.setMoreResults(true); + builder.setMoreResults(false); builder.addCellsPerResult(r.size()); final List cellScannables = new ArrayList(1); cellScannables.add(r); @@ -1209,7 +1209,7 @@ public class TestAssignmentManager { // Get a meta row result that has region up on SERVERNAME_A for REGIONINFO Result r = MetaMockingUtil.getMetaTableRowResult(REGIONINFO, SERVERNAME_A); final ScanResponse.Builder builder = ScanResponse.newBuilder(); - builder.setMoreResults(true); + builder.setMoreResults(false); builder.addCellsPerResult(r.size()); final List rows = new ArrayList(1); rows.add(r); @@ -1229,8 +1229,9 @@ public class TestAssignmentManager { .thenAnswer(ans).thenAnswer(ans).thenAnswer(ans).thenAnswer(ans).thenAnswer(ans) .thenReturn(ScanResponse.newBuilder().setMoreResults(false).build()); } else { - Mockito.when(ri.scan((RpcController) Mockito.any(), (ScanRequest) Mockito.any())).thenAnswer( - ans); + Mockito.when(ri.scan((RpcController) Mockito.any(), (ScanRequest) Mockito.any())) + .thenAnswer(ans).thenAnswer(ans).thenAnswer(ans).thenAnswer(ans) + .thenReturn(ScanResponse.newBuilder().setMoreResults(false).build()); } // If a get, return the above result too for REGIONINFO GetResponse.Builder getBuilder = GetResponse.newBuilder(); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java index 473946c..2c8b46d 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java @@ -167,6 +167,8 @@ public class TestRegionServerMetrics { // Adding some meta related requests requests += 3; readRequests ++; + // Extra request after observing ScanResponse.getMoreResults + requests++; metricsRegionServer.getRegionServerWrapper().forceRecompute(); metricsHelper.assertCounter("totalRequestCount", requests + 50, serverSource); -- 2.1.2