Skip to content

Make zookeeper version adjustable (ZK_VERSION env)#4734

Merged
sougou merged 1 commit intovitessio:masterfrom
adsr:patch-zk-version
Mar 31, 2019
Merged

Make zookeeper version adjustable (ZK_VERSION env)#4734
sougou merged 1 commit intovitessio:masterfrom
adsr:patch-zk-version

Conversation

@adsr
Copy link
Copy Markdown
Collaborator

@adsr adsr commented Mar 18, 2019

This patch parameterizes the ZooKeeper version. Confirmed working with ZK_VERSION=3.5.4-beta. There is a comment about updating the Dockerfile but I'm not so familiar with that.

Not included in this patch but related:

diff --git a/config/zkcfg/zoo.cfg b/config/zkcfg/zoo.cfg
index 8b1b00994..b8f811706 100644
--- a/config/zkcfg/zoo.cfg
+++ b/config/zkcfg/zoo.cfg
@@ -1,9 +1,10 @@
 tickTime=2000
 dataDir={{.DataDir}}
 clientPort={{.ClientPort}}
 initLimit=5
 syncLimit=2
 maxClientCnxns=0
 {{range .Servers}}
 server.{{.ServerId}}={{.Hostname}}:{{.LeaderPort}}:{{.ElectionPort}}
 {{end}}
+4lw.commands.whitelist=conf,cons,crst,dump,envi,ruok,srst,srvr,stat,wchs,mntr

The default config for 4lw.commands.whitelist is different in 3.5+. The above diff explicitly sets it to the default of 3.4. Without this, the ruok healthcheck fails. Alternatives: (a) parameterize (e.g., {{.FourLetterWords}}), (b) ability to add arbitrary config, (c) something else?

@adsr adsr requested a review from sougou as a code owner March 18, 2019 21:28
@adsr adsr force-pushed the patch-zk-version branch from 2375ca6 to a25e7a8 Compare March 18, 2019 21:30
@adsr adsr force-pushed the patch-zk-version branch 2 times, most recently from f794b18 to fb3f922 Compare March 25, 2019 22:01
Signed-off-by: Adam Saponara <as@php.net>
@adsr adsr force-pushed the patch-zk-version branch from fb3f922 to c15d629 Compare March 26, 2019 18:20
@sougou sougou merged commit 7c23d08 into vitessio:master Mar 31, 2019
@rafael
Copy link
Copy Markdown
Member

rafael commented Apr 2, 2019

We should also add installation of ant to bootstrap_vm.sh in vagrant scripts. So Vagrant setup does not break after merging this.

@rafael rafael mentioned this pull request Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants