HBASE-28428 : Zookeeper ConnectionRegistry APIs should have timeout #5837
Conversation
This file contains hidden or 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. |
Apache9
requested changes
Apr 19, 2024
Contributor
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
This is not the correct way, as we place lots of boiler plate code everywhere...
Please add the timeout control in connection registry implementation. DIfferent connection registry implementations can have their own way to implement the timeout.
| } | ||
|
|
||
| }); | ||
| }, conf.getInt(HConstants.CONNECTION_REGISTRY_API_TIMEOUT, |
Contributor
There was a problem hiding this comment.
I do not think this is the correct place to put these timeout configurations.
We should place them into the connection registry implementation.
Contributor
|
@Apache9 how do we ensure the timeout is considered by CompletableFuture? ConnectionRegistry APIs return CompletableFuture so if we implement timeout on CompletableFuture as part of connection registry implementation, then in reality all APIs would become synchronous and return actual values rather than wrapped with CompletableFuture right? |
Contributor
|
You can see how we implement timeout in AsyncRpcConnectionImpl... There is a timer, when we reach timeout, the timer task will call completeExceptionally method of the CompletableFuture, so the upper layer will get a timeout exception. So for rpc based connection registry, we could set the operation timeout while doing the request, and for zk based, we should introduce a timer to do this work. Thanks. |
Contributor
Author
|
@Apache9 where can I find the AsynRpcConnectionImpl code? We tried finding it but are having no luck in the master code base. Thank you |
Contributor
|
@Divneet18 here is the sample for TimerTask and how we can introduce timeout: |
Contributor
|
The other example is for |
Contributor
|
Here, |
Contributor
|
Have you guys find the way to use timer to implement the timeout? And for ZKConnectionRegistry, you can pass the TIMER in AsyncConnectionImpl to the ReadOnlyZKClient, and use it in ReadOnlyZKClient. More specific, you can schedule a timer task when calling the exec method for ZKTask, and once we reach the timeout time, we call completeExceptionally on the future in ZKTask. Thanks. |
|
💔 -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. |
virajjasani
reviewed
May 14, 2024
|
|
||
| private static final long expectedTimeout = 120000; | ||
| private static final int maxAttempts = 5; | ||
| private static final long pauseNs = 100000; |
Contributor
There was a problem hiding this comment.
These should be configs, not constants.
Apache9
requested changes
May 14, 2024
Contributor
There was a problem hiding this comment.
The implementation is incorrect...
The problem here is the async zk operation does not have timeout, it may depends on the session timeout configuration but not very stable, so we need to use an external timer task to force finish the completable future.
Here you just submit the async zk operation in the timer task, and then in the callback, you check reschedule the timer task and then you have a chance to check timeout, but what if the zk operation does not return within the timeout?...
|
|
||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import org.apache.hadoop.conf.Configuration; |
| @Override | ||
| public RegionLocations getRegionLocations(TableName tableName, int replicaId, | ||
| boolean reload) throws Exception { | ||
| final Configuration conf = HBaseConfiguration.create(); |
Contributor
There was a problem hiding this comment.
Please remove this useless code?
|
|
||
| private static AsyncClusterConnection createAsyncClusterConnection(Configuration conf, | ||
| ConnectionRegistry registry, SocketAddress localAddress, User user) throws IOException { | ||
| ConnectionRegistry registry, SocketAddress localAddress, User user ) throws IOException { |
Contributor
@Apache9 this is exactly why i was initially proposing to use CompletableFuture#get with timeout in all callers so that regardless of the ConnectionRegistry implementation, we can still have our timeout at Future object level. |
Contributor
I've already mentioned above using CompletableFuture.get is incorrect, the jira title is ConnectionRegistry APIs should have timeout, not the caller should have timeout, and how do you plan to deal with async operations? You should never do future.get in an async operation. |
Contributor
|
Both approaches have their own drawbacks. |
Contributor
The timer is used to complete the CompletableFuture when the zookeeper does not respond in time... |
Contributor
Oh wait, we need TimeTask for |
Contributor
In ZKConnetionRegistry, you have a CompletableFuture object returned to upper layer, and you will issue a zk async operation, in the callback, you will complete the CompletableFuture object so the upper layer will know. Now the problem is you can not control the timeout of zk async operations, so when issue a zk async operation, you need to set up a timer task at the same time, and once the timer task is executed, you try to complete the CompletableFuture with a TimeoutException if it has not been completed yet, so the upper layer will know the operation failed. |
Contributor
|
@Apache9, I looked into this with Divneet today. How about this method in At ZKConnectionRegistry, we can call zk#getWithTimeout rather than zk#get. |
Contributor
|
@Divneet18 could you please give this a shot? |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@Apache9 The methods without timeout are only being called in methods with timeout and one test file. So is it okay to not remove them? |
Contributor
|
Please fix the spotless error? Just run 'mvn spotless:apply', I think the PR is almost ready, let's get a clean pre commit build result first. Thanks @Divneet18 for your patience. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@Apache9 @virajjasani we have a clean commit now. unless something else needs to be changed? |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
javac warning does not seem relevant |
Contributor
|
Let's wait for @Apache9 |
Apache9
approved these changes
Jul 16, 2024
Contributor
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
In general LGTM. Only some small nits.
Thanks.
| private static int CHILDREN = 5; | ||
|
|
||
| private static ReadOnlyZKClient RO_ZK; | ||
| static final HashedWheelTimer RETRY_TIMER = new HashedWheelTimer( |
Contributor
There was a problem hiding this comment.
Pealse use 'private static final' since we do not need to expose it to other classes.
And please shutdown it in afterClass method.
Contributor
There was a problem hiding this comment.
@Divneet18 please take care of this and you can also start preparing branch-2 PR after addressing this.
virajjasani
changed the title
pankaj72981
approved these changes
Jul 16, 2024
Contributor
pankaj72981
left a comment
pankaj72981
left a comment
There was a problem hiding this comment.
Overall changes LGTM, apart from the nits.
hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ReadOnlyZKClient.java
Outdated
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. |
virajjasani
pushed a commit
that referenced
this pull request
Jul 18, 2024
…5837) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
virajjasani
pushed a commit
that referenced
this pull request
Jul 19, 2024
…6095) (#5837) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
virajjasani
pushed a commit
that referenced
this pull request
Jul 19, 2024
…6095) (#5837) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
virajjasani
pushed a commit
that referenced
this pull request
Jul 19, 2024
…6095) (#5837) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Jira: HBASE-28428