-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[Issue 4480] Support setting zk chroot path when initialize cluster metadata #4502
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
Conversation
@@ -93,7 +92,7 @@ | |||
private String globalZookeeper; | |||
|
|||
@Parameter(names = { "-cs", | |||
"--configuration-store" }, description = "Configuration Store connection string", required = false) | |||
"--configuration-store" }, description = "Configuration Store connection string", required = true) |
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.
Hi @murong00, Is this change necessary?
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.
If someone missed this option, it will print error Configuration store address argument is required (--configuration-store)
since --global-zookeeper
option is hidden . So it may be better to make this option required
.
@@ -273,6 +270,29 @@ public static void main(String[] args) throws Exception { | |||
log.info("Cluster metadata for '{}' setup correctly", arguments.cluster); | |||
} | |||
|
|||
private static ZooKeeper initZk(String connection, int sessionTimeout) throws Exception { |
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.
@murong00 can you rewrite unit tests for this method?
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.
Added.
@jiazhai can you review this again? |
Motivation
Fixes #4480
Modifications
Create the zookeeper chroot path at first if it doesn't exist.