-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-26784 Use HIGH_QOS for ResultScanner.close requests #4146
Conversation
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
… of a scan prematurely
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
0f6b831
to
712abb6
Compare
the test failure was a flake. I just force-pushed a no-op change to kick off the build again. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other ways to ensure the execution of a scanner close request? For instance, what if the CallRunner
skipped checking for client disconnect and call timeouts for close requests?
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableRpcPriority.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/client/AbstractTestAsyncTableScan.java
Outdated
Show resolved
Hide resolved
@@ -188,6 +188,7 @@ private void startScan(OpenScannerResponse resp) { | |||
private CompletableFuture<OpenScannerResponse> openScanner(int replicaId) { | |||
return conn.callerFactory.<OpenScannerResponse> single().table(tableName) | |||
.row(scan.getStartRow()).replicaId(replicaId).locateType(getLocateType(scan)) | |||
.priority(scan.getPriority()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This absence was a bug, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was a bug. I caught it in my investigation into these tests. Should I be filing a separate JIRA for that, or is it ok to include here?
Thanks for the review @ndimiduk
Are you opposed this change as is, or are you suggesting the CallRunner change in addition? It's a good question, but I think it gets tricky to rely solely on that. For example, what if the call queue is full? So then we go down the path of dropping enqueued requests to make room for close requests. It felt cleaner to rely on priorities, as we do for ensuring that meta requests for example go to high priority. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.times; | ||
import static org.mockito.Mockito.verify; | ||
import static org.mockito.Mockito.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid star imports.
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
This PR should be all set -- There is 1 unrelated test failure which looks like a CI issue. I ran the test (TestSyncReplicationActive) locally just to be sure and it looks good:
|
This patch applies cleanly to branch-2 as well, but we need to also update the blocking client there so I'm submitting a separate PR.