-
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-28509 ScanResumer.resume would perform unnecessary scan when cl… #5817
Conversation
…ose AsyncTableResultScanner
🎊 +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. |
* <p> | ||
* You are free to call it multiple time but only the first call will take effect. | ||
*/ | ||
default void terminate() { |
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 class is called ScanResumer, so I do not think we should add a terminate method to it, it will increase the complexity for our users to implement scanner. If there are enough results, the users could just call ScanController.terminate, instead of suspending the scanner and then terminate it later...
Instead, I think we could introduce a IA.Private interface, or just expose the implementation class as package private, and in the close method of AsyncTableResultScanner, we check if the ScanConsumer is this class, if so we cast it to the implementation class and call the terminate method there, without exposing a terminate method in ScanResumer.
WDYT?
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.
@Apache9, thank you for review, ok, I think your opinion is better and change the code.
🎊 +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. |
🎊 +1 overall
This message was automatically generated. |
@@ -143,6 +144,18 @@ private void resumePrefetch() { | |||
resumer = null; | |||
} | |||
|
|||
private void termianteResumerIfPossible() { |
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.
Typo? terminateResumerIfPossible.
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.
Ok, I have fixed it.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableResultScanner.java
Show resolved
Hide resolved
🎊 +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. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
…ose AsyncTableResultScanner