ndimiduk commented on code in PR #5402:
URL: https://github.com/apache/hbase/pull/5402#discussion_r1330297043
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestClientScannerTimeouts.java:
##########
@@ -174,18 +178,27 @@ public void
testRetryOutOfOrderScannerNextExceptionAsync() throws IOException {
* timed out next() call and mess up the test.
*/
@Test
- public void testNormalScanTimeoutOnNext() throws IOException {
+ public void testNormalScanTimeoutOnNextWithLegacyMode() throws IOException {
Review Comment:
Can you call this something other than legacy mode? The word "legacy" is
always relative, by definition.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionConfiguration.java:
##########
@@ -76,6 +76,11 @@ public class ConnectionConfiguration {
public static final String HBASE_CLIENT_META_SCANNER_TIMEOUT =
"hbase.client.meta.scanner.timeout.period";
+ public static final String HBASE_CLIENT_USE_SCANNER_TIMEOUT_FOR_NEXT_CALLS =
+ "hbase.client.use.scanner.timeout.for.next.calls";
Review Comment:
Our scanner timeout configuration called
`hbase.client.scanner.timeout.period`, so maybe this one should be
`hbase.client.use.scanner.timeout.period.for.next.calls`. I dunno. Maybe it's
clear enough as it is? What do you think?
Whatever its called, please add a comment about this new flag in
https://hbase.apache.org/book.html#config_timeouts
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestClientScannerTimeouts.java:
##########
@@ -101,6 +102,9 @@ public static void setUpBeforeClass() throws Exception {
conf.setInt(HConstants.HBASE_CLIENT_SCANNER_TIMEOUT_PERIOD, scanTimeout);
conf.setInt(HBASE_CLIENT_META_READ_RPC_TIMEOUT_KEY, metaScanTimeout);
conf.setInt(HBASE_CLIENT_META_SCANNER_TIMEOUT, metaScanTimeout);
+ // set to true by default here. it only affects next() calls, and we'll
explicitly
+ // set it to false in one of the tests below to test legacy behavior for
next call
+ conf.setBoolean(HBASE_CLIENT_USE_SCANNER_TIMEOUT_FOR_NEXT_CALLS, true);
Review Comment:
This is perhaps too subtle -- we're no longer testing with the default
configuration except for a single test. I'd have more confidence in the
coverage if, for example, this class was parameterized on this config flag.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionConfiguration.java:
##########
@@ -76,6 +76,11 @@ public class ConnectionConfiguration {
public static final String HBASE_CLIENT_META_SCANNER_TIMEOUT =
"hbase.client.meta.scanner.timeout.period";
+ public static final String HBASE_CLIENT_USE_SCANNER_TIMEOUT_FOR_NEXT_CALLS =
+ "hbase.client.use.scanner.timeout.for.next.calls";
Review Comment:
Further troubling, configuration points defined here are not populated into
the online book automatically. I think you need to add the config, a default,
and a description to hbase-defaults.xml in order for it to automatically gain
mention in the book.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java:
##########
@@ -116,16 +117,15 @@ public ClientScanner(final Configuration conf, final Scan
scan, final TableName
this.connection = connection;
this.pool = pool;
this.primaryOperationTimeout = primaryOperationTimeout;
- this.retries = conf.getInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER,
Review Comment:
👍
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallableWithReplicas.java:
##########
@@ -72,11 +73,12 @@ class ScannerCallableWithReplicas implements
RetryingCallable<Result[]> {
public ScannerCallableWithReplicas(TableName tableName, ClusterConnection
cConnection,
ScannerCallable baseCallable, ExecutorService pool, int
timeBeforeReplicas, Scan scan,
- int retries, int readRpcTimeout, int scannerTimeout, int caching,
Configuration conf,
- RpcRetryingCaller<Result[]> caller) {
+ int retries, int readRpcTimeout, int scannerTimeout, boolean
useScannerTimeoutForNextCalls,
Review Comment:
Is it better to thread through the instance of `ConnectionConfiguration`
rather than this new boolean flag? Probably many of these fields can be folded
down into an inexpensive field lookup from the `ConnectionConfiguration`
instance instead.
Probably this is more of a refactor than you were ready to bite off, and all
this code gets tossed as of 3.0, yes?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java:
##########
@@ -116,16 +117,15 @@ public ClientScanner(final Configuration conf, final Scan
scan, final TableName
this.connection = connection;
this.pool = pool;
this.primaryOperationTimeout = primaryOperationTimeout;
- this.retries = conf.getInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER,
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
