-
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-27902 Utility to invoke coproc on multiple servers using AsyncAdmin #5295
Conversation
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Thanks @jinggou checkstyle and spotless need fixes |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +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.
+1
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.
I still do not think we need to add this method in the Admin interface, it is very easy to implement by our users. And even if we really want to add this support, a default method at the AsyncAdmin interface is enough? It can be done through public methods in AsyncAdmin interface...
Thanks Duo, i understand your point but using FutureUtil utilities by client applications is not recommended given it is IA#Private.
fair enough, let me check this, thank you! |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
What about introducing an IA.Public class called AsyncAdminClientUtils, and put this method there? |
sorry, i didn't get it, which method? btw @jinggou brought the implementation to the default method in AsyncAdmin, this should also be good? WDYT? |
Just like this
|
oh i see, but even same is fine with AsyncAdmin also since it is already IA#Public? but otherwise for this particular method, we are good with default implementation on AsyncAdmin as well right? we don't need AsyncAdminClientUtils because AsyncAdmin is already IA#Public, correct?
this is the current state of this PR and master branch PR #5266 |
AsyncAdmin is a more critical API, so do not want to add unnecessary methods to it. As I said at the very beginning, this could be done by combining with several methods in AsyncAdmin, which means it is redundant. But it is fine to introduce a helper class to put this method, to make our users write less code. That's my point. |
Alright, as long as we have IA#Public API for this, it is fine, no problem with having new class: AsyncAdminClientUtils as IA.Public |
…iceOnAllRegionServers method
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -56,6 +56,7 @@ public void testAdminWithAsyncAdmin() { | |||
adminMethodNames.remove("getConfiguration"); | |||
adminMethodNames.removeAll(getMethodNames(Abortable.class)); | |||
adminMethodNames.removeAll(getMethodNames(Closeable.class)); | |||
asyncAdminMethodNames.remove("coprocessorServiceOnAllRegionServers"); |
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.
same as master branch PR, we might not need this change anymore
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.
+1.
@@ -56,6 +56,7 @@ public void testAdminWithAsyncAdmin() { | |||
adminMethodNames.remove("getConfiguration"); | |||
adminMethodNames.removeAll(getMethodNames(Abortable.class)); | |||
adminMethodNames.removeAll(getMethodNames(Closeable.class)); | |||
asyncAdminMethodNames.remove("coprocessorServiceOnAllRegionServers"); |
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.
+1.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
…dmin (#5295) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
…dmin (#5295) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
…dmin (apache#5295) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 8c50433) Change-Id: I92c830ac6f4f5726561b66d5ec2bee86332df5df
No description provided.