HBASE-27902 Utility to invoke coproc on multiple servers using AsyncAdmin #5266
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
|
💔 -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. |
|
Have you guys read my comment on the jira issue? In the async admin implementation, I do not think we need a thread pool for executing the coprocessor calls for each server... |
|
The code should be just like this. The return value is a Map to map the region server to its result, I use Object so it could also represent the failure when requesting a region server, instead of failing the whole CompletableFuture, so later you can check the returned map and find out the failed region servers and try to send requests again. |
|
Oh, you have passed the server names in... Then it will easier, just move the code in the first level of callback out is enough... |
correct, we don't even need call to |
|
Thanks @Apache9 ! I've made code changes. Please take a look. |
|
💔 -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. |
shahrs87
reviewed
Jun 7, 2023
| } else { | ||
| resultMap.put(rs, r); | ||
| } | ||
| done = resultMap.size() == serverNames.size(); |
|
I still have concerns about the necessity for introducing this method, especially that if you pass the list of server names in and get a List of CompletableFuture back, why not just write code by our own? |
|
@Apache9 i understand your point, maybe we can update existing method and use List instead of single ServerName and that is more flexible as client can pass single server as well, correct? but since we already have that method since 2.0, we will have to go through deprecation cycle (maybe we still can). However given that FutureUtils have bunch of nice utilities worth using by methods like this one, and it is IA.Private, it would be nice to have it in hbase IMHO than let client use CompletableFuture callbacks/whenCompleted etc. better to let the client only worry about using if you are fine, i can now start reviewing. please let me know. Thanks |
what if we have one more argument our usecase is anyways to be able to run on all servers but just to make it look more flexible for any other client usecases, we thought of using list of servers. if Duo's opinion remains same, there is nothing wrong with removing |
virajjasani
reviewed
Jun 8, 2023
...test/java/org/apache/hadoop/hbase/coprocessor/TestAsyncRegionServersCoprocessorEndpoint.java
Outdated
Show resolved
Hide resolved
|
@jinggou could you please update this to not include list of servers? let's go with Duo's suggestion and let the api internally get server list and also pass it to listener util of FutureUtils (similar to Duo's suggestion at the beginning). |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
approved these changes
Jun 14, 2023
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
As replied on the branch-2 PR, I still have concerns about whether we should introduce the method in our AsyncAdmin interfaces, as it is easy to implement this method through the combination of public methods in the AsyncAdmin interface... |
|
since using FutureUtil utilities by clients directly is not recommended, let's keep this logic in AsyncAdmin as "default" implementation rather than keeping it in implementing classes |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…iceOnAllRegionServers method
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
reviewed
Jun 20, 2023
| @@ -58,6 +58,7 @@ public void testAdminWithAsyncAdmin() { | |||
| adminMethodNames.removeAll(getMethodNames(Closeable.class)); | |||
|
|
|||
| asyncAdminMethodNames.remove("coprocessorService"); | |||
| asyncAdminMethodNames.remove("coprocessorServiceOnAllRegionServers"); | |||
There was a problem hiding this comment.
do we need this change anymore?
There was a problem hiding this comment.
We do not need this any more
virajjasani
changed the title
Apache9
approved these changes
Jun 20, 2023
| @@ -58,6 +58,7 @@ public void testAdminWithAsyncAdmin() { | |||
| adminMethodNames.removeAll(getMethodNames(Closeable.class)); | |||
|
|
|||
| asyncAdminMethodNames.remove("coprocessorService"); | |||
| asyncAdminMethodNames.remove("coprocessorServiceOnAllRegionServers"); | |||
There was a problem hiding this comment.
We do not need this any more
virajjasani
approved these changes
Jun 20, 2023
|
will merge once pending Jenkins QA is done |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
pushed a commit
that referenced
this pull request
Jun 20, 2023
…dmin (#5266) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
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.
No description provided.