HBASE-25903 ReadOnlyZKClient APIs - CompletableFuture.get() calls can cause threads to hang forver when ZK client create throws Non IOException #3293
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
… cause threads to hang forver when ZK client create throws Non IOException
Apache9
requested changes
May 24, 2021
| @@ -344,6 +344,10 @@ private void run() { | |||
| } catch (IOException e) { | |||
| task.connectFailed(e); | |||
| continue; | |||
| } catch (Exception e) { | |||
| // won't be doing any retries for non IOE. | |||
| task.closed(e); | |||
There was a problem hiding this comment.
Why call closed here? This is a connect failed?
There was a problem hiding this comment.
Actually in connect fail case, zk is supposed to throw IOE and we handle that above. The connectFailed will cause the retries. As mentioned in Jira, some old versions of zk has a bug and cause a RuntimeException while zk hostname resolve. So as such, on our dev branches this issue wont happen as we moved to zk 3.5.7
Still I think its better to handle here. Else we will end up hanging the Future.get() call ever. In my cluster version where zk client is old, I see this causes ReplicationSource init to hang forver and WALs getting accumulated and replication lag. Later when we see HDFS disk alerts, then only analyzed. the culprit was DNS which gives temp hostname resolution issues.
I thought like whether we should catch Exception instead of IOE in 1st place and do connectFailed only so that we will get retry. But just changed my mind later because its generic Exception we catch. Theoretically its IOE what we have to retry right? Still am open for change. There is nothing wrong if do few more retries before give up.
BTW in my internal version, i will handle the latter way as I can not upgrade zk client version that easily.
Thoughts Duo?
There was a problem hiding this comment.
@Apache9 So u prefer handle any Exception as connect failed and so retry? Am ok for that too.
There was a problem hiding this comment.
FWIW, if you think this is an unrecoverable error, we should just quit the whole ReadOnlyZKClient loop, instead of calling closed here but do not break out the loop? If not, why give up retry at the first time?
There was a problem hiding this comment.
I got ur view Duo. Thanks. Ya I took a middle approach. Thought about this also.
I am very much fine for handling Exception as how we do IOE and do connectFailed(). After retry anyways will come out. Ideally the exception will be IOE. I read like u also okf with that. I will make that change.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
… cause threads to hang forver when ZK client create throws Non IOException
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
May 30, 2021
anoopsjohn
added a commit
that referenced
this pull request
May 31, 2021
… cause threads to hang forver when ZK client create throws Non IOException (#3293) Signed-off-by: zhangduo <[email protected]>
anoopsjohn
added a commit
that referenced
this pull request
May 31, 2021
… cause threads to hang forver when ZK client create throws Non IOException (#3293) Signed-off-by: zhangduo <[email protected]>
anoopsjohn
added a commit
that referenced
this pull request
May 31, 2021
… cause threads to hang forver when ZK client create throws Non IOException (#3293) Signed-off-by: zhangduo <[email protected]>
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.