Support mechanism to provide external zookeeper-server list to build global/configuration zookeeper #269
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
…global/configuration zookeeper
MarvinCai
reviewed
Jun 20, 2022
| {{- $global := . }} | ||
| {{ range $i, $e := until (.Values.zookeeper.replicaCount | int) }}{{ if ne $i 0 }},{{ end }}{{ template "pulsar.fullname" $global }}-{{ $global.Values.zookeeper.component }}-{{ printf "%d" $i }}{{ end }} | ||
| - name: ZOOKEEPER_SERVERS | ||
| {{- if .Values.zookeeper.configData.ZOOKEEPER_SERVERS }} |
There was a problem hiding this comment.
probably add it to value file with an example and comment it out? so user can know how to use this feature.
As from the script change, seems the provided server list need to contain election port and follower port.
There was a problem hiding this comment.
@rdhabalia - are you able to update the values.yaml file so that this configuration is documented? I plan to release an updated version of the helm chart next week, and it'd be great to include this.
There was a problem hiding this comment.
yes, I have added sample-example values in examples/values-zookeeper-aws.yaml and values.yaml
michaeljmarshall
approved these changes
Oct 18, 2022
|
I missed a minor detail with how the feature is configured. I proposed a fix here #308. |
michaeljmarshall
added a commit
that referenced
this pull request
Oct 19, 2022
### Motivation In #269, we added a way to configure external zookeeper servers. However, it was added to the wrong section of the zookeeper config. The `zookeeper.configData` section is mapped directly into the zookeeper configmap. ### Modifications Move `zookeeper.configData.ZOOKEEPER_SERVERS` to `zookeeper.externalZookeeperServerList` ### Verifying this change This is a cosmetic change on an unreleased feature.
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.
Fixes #268
Motivation
Right now, chart dynamically creates zk cluster with zk pods initialized in the same namespace. However, for global/configuration zookeeper, user requires to build zk clusters with pods deployed in different namespaces. Therefore, user needs a mechanism to pass an external list of zk-servers to the chart and build zk-cluster with pods across different namespaces.
Modification
This PR sets
ZOOKEEPER_SERVERSvalue provided by user and also sets override-value flag which will be used by generate-zookeeper-config.sh to override external zk list in config file and assign appropriate id to the host.apache/pulsar#15987 fixes generate-zookeeper-config.sh changes.
Result
ZOOKEEPER_SERVERSstring intozookeeper.configDatain Values.yaml file to override external zk-server list.