HBASE-28563 Closing ZooKeeper in ZKMainServer #5869
Merged
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. |
anmolnar
approved these changes
May 3, 2024
There was a problem hiding this comment.
lgtm. Just a few nitpicks.
The problem of non-daemon Netty threads has probably already been resolved by:
ZOOKEEPER-4804 Use daemon threads for Netty client
and the original HACK is probably not needed either, but otherwise looks okay.
Comment on lines
45
to
46
| private static class HACK_UNTIL_ZOOKEEPER_1897_ZooKeeperMain extends ZooKeeperMain | ||
| implements Closeable { |
There was a problem hiding this comment.
Probably not scope of this ticket, but ZOOKEEPER-1897 resolved long ago. Do we still need this class?
There was a problem hiding this comment.
We have bumped our zookeeper dependency to 3.8.x, do we still need this?
There was a problem hiding this comment.
Thank you for reviewing the code and thank you for mentioning ZOOKEEPER-4804.
I did not know that non-daemon Netty threads issue had been resolved in ZooKeeper.
It seems that the HACK_UNTIL_ZOOKEEPER_1897_ZooKeeperMain class is no longer necessary.
Instead of using HACK_UNTIL_ZOOKEEPER_1897_ZooKeeperMain, it might be better to use ZooKeeperMain directly.
ZooKeeperMain also closes ZooKeeper when a timeout occurs.
However, it seems that the ZOOKEEPER-4804 patch is not applied in the 3.8.4 version used by HBase.
Therefore, in order to remove HACK_UNTIL_ZOOKEEPER_1897_ZooKeeperMain, it appears that a ZooKeeper version upgrade(3.8.4 -> 3.9.x) is necessary.
Two options are suggested below. Which option would be the most appropriate?
- Upgrade ZooKeeper to version 3.9.x and remove
HACK_UNTIL_ZOOKEEPER_1897_ZooKeeperMain.
Is it safe to upgrade ZooKeeper to version 3.9.x? I am concerned that other issues may arise. - Always ensure
HACK_UNTIL_ZOOKEEPER_1897_ZooKeeperMainperforms closing. (Current approach)
There was a problem hiding this comment.
Better also add ZOOKEEPER-4804 in the comment so others will know why we still need this hack.
There was a problem hiding this comment.
Thank you for your advice.
I have added ZOOKEEPER-4804 to the comments.
There was a problem hiding this comment.
Sounds good to me, thanks.
There was a problem hiding this comment.
I'm pleased to inform that the code review has been approved and the CI checks have passed successfully.
If possible, would you kindly merge this code? I apologize for any inconvenience, but I don't have the necessary permissions to perform the merge myself.
Alternatively, if there are any additional steps required to proceed with the merge, please advise me.
| @@ -61,6 +61,7 @@ public static ZooKeeper ensureConnectedZooKeeper(ZooKeeper zookeeper, int timeou | |||
| while (!zookeeper.getState().isConnected()) { | |||
| Threads.sleep(1); | |||
| if (stopWatch.elapsed(TimeUnit.MILLISECONDS) > timeout) { | |||
| zookeeper.close(); | |||
There was a problem hiding this comment.
I suspect you added this to cover the case when constructor of HACK_UNTIL_ZOOKEEPER_1897_ZooKeeperMain failes and close() not getting called. I think you might want to do this within the constructor by putting ZooKeeperHelper.ensureConnectedZooKeeper(this.zk, 30000); in a try-catch block to keep responsibility inside the class.
There was a problem hiding this comment.
Thank you for reviewing
I have changed the code to handle ZooKeeperConnectionException in the constructor.
|
🎊 +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. |
Apache9
approved these changes
May 6, 2024
|
🎊 +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
pushed a commit
that referenced
this pull request
May 8, 2024
Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: Andor Molnár <andor@apache.org> (cherry picked from commit 23fa363)
Apache9
pushed a commit
that referenced
this pull request
May 8, 2024
Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: Andor Molnár <andor@apache.org> (cherry picked from commit 23fa363)
Apache9
pushed a commit
that referenced
this pull request
May 8, 2024
Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: Andor Molnár <andor@apache.org> (cherry picked from commit 23fa363)
Apache9
pushed a commit
that referenced
this pull request
May 8, 2024
Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: Andor Molnár <andor@apache.org> (cherry picked from commit 23fa363)
Apache9
pushed a commit
that referenced
this pull request
May 8, 2024
Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: Andor Molnár <andor@apache.org> (cherry picked from commit 23fa363)
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
May 21, 2024
Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: Andor Molnár <andor@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.
HBASE-28563
closeto HACK_UNTIL_ZOOKEEPER_1897_ZooKeeperMain