HBASE-28085 Configurably use scanner timeout as rpc timeout for scanner next calls #5402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
bbeaudreault
force-pushed
the
HBASE-28085
branch
from
cf938ac to
1dc500c
Compare
bbeaudreault
commented
Sep 14, 2023
| @@ -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, | |||
There was a problem hiding this comment.
While I was here and needed a new config, I cleaned these up. Configuration getter calls introduce synchronization and parse cost, so it's better to use what we already parsed in ConnectionConfiguration.
bbeaudreault
force-pushed
the
HBASE-28085
branch
from
1dc500c to
716a19c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
approved these changes
Sep 19, 2023
There was a problem hiding this comment.
Looks alright to me. Threading through all these method parameters (int, int, int, int, now bool too) makes me cringe. If you expect users to make use of this flag as part of their transition to 3.0 and async config semantics, please make mention of it in the book.
| @@ -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, | |||
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestClientScannerTimeouts.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestClientScannerTimeouts.java
Outdated
Show resolved
Hide resolved
| @@ -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"; | |||
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this comment.
I changed the property name as requested. I added https://issues.apache.org/jira/browse/HBASE-28098 to update book.
I'm not sure we should add this to hbase-default.xml. It's more of a feature flag, and I think of hbase-default.xml as holding defaults for common configs. I dunno though, we are very inconsistent there and im not sure of the criteria for adding (many many many configs are missing from it).
| @@ -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, | |||
There was a problem hiding this 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?
There was a problem hiding this comment.
Actually, it's not so simple here. The readRpcTimeout and scannerTimeout both can be overridden on a per-Table basis via TableBuilder. Caching can be overridden by the Scan. So really just retries and my new boolean could be unified in this way. I could do it, but yea trying not to refactor too much. Also I could imagine making retries configurable via TableBuilder in the future which would negate this change... not necessarily somethign we should design for, just saying.
Yes, this code goes away in 3.0, but I'm sure we'll be maintaining it for years to come. So I think it's worth doing useful refactors, but not sure this one qualifies given the special cases above.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Sep 20, 2023
|
I pushed a small addition for completeness. Will merge once the pre-commit comes back green. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Spotless succeeded but failed with a process-related issue at the end. I checked it locally as well just in case. Tests are passing. Merging. |
bbeaudreault
added a commit
to HubSpot/hbase
that referenced
this pull request
Sep 20, 2023
… timeout for scanner next calls (apache#5402) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
bbeaudreault
added a commit
to HubSpot/hbase
that referenced
this pull request
Sep 22, 2023
… timeout for scanner next calls (apache#5402) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a new
hbase.client.use.scanner.timeout.for.next.callsconfig, defaulting to false. When true, ClientScanner (for HTable) will use the scanner timeout for next() calls like AsyncTable does. As with AsyncTable, other calls (openScanner, closeScanner, renewLease) continue to use readRpcTimeout.Adds a new test in TestClientScannerTimeouts to verify old and new behavior.
I wonder if we should enable this by default in branch-2, and disable by default in 2.5.x. Open to opinions.