feat:support kafka 2.7 using external zookeeper #1297
Conversation
lancelot1989
requested review from
caiq1nyu,
leon-inf,
Y-Rookie and
a team
as code owners
|
thanks for your PR. we will review it ASAP. |
| runAsNonRoot: true | ||
| runAsUser: 1001 | ||
| command: | ||
| {{/* - sleep*/}} |
There was a problem hiding this comment.
It is better to remove the test code :)
There was a problem hiding this comment.
fixed，thanks for your remind
| fieldRef: | ||
| apiVersion: v1 | ||
| fieldPath: status.hostIP | ||
| - name: KAFKA_CFG_PROCESS_ROLES |
There was a problem hiding this comment.
'Process role' is a concept in the kraft architecture. For 'cmpd-broker' of Kafka 2.7, this component is actually a certain 'broker', and this environment variable is not needed.
| # Represents the endpoint of the service connection credential. | ||
| endpoint: | ||
| # your external zk endpoints here | ||
| value: "zookeeper-cluster-zookeeper-0.zookeeper-cluster-zookeeper-headless.default:2181,zookeeper-cluster-zookeeper-1.zookeeper-cluster-zookeeper-headless.default:2181,zookeeper-cluster-zookeeper-2.zookeeper-cluster-zookeeper-headless.default:2181/test" |
There was a problem hiding this comment.
LGTM. However, for scenarios where multiple clusters share zk, can we consider entrusting the control logic of metadata conflicts to addon instead of relying on specifying '/test' when defining ServiceDescriptor?
For example:
- the only thing defined in ServiceDescriptor is the zk base address:
endpoint:
# your external zk endpoints here
value: "zk-1.zookeeper-cluster-zookeeper-headless.default:2181"
- an env defined in cmpd-broker such as: ZK_SUB_PATH=kafka1, the default value is KB_CLUSTER_NAME. Finally, in broker-env.tpl, KB_KAFKA_ZOOKEEPER_CONN will process into 'zk-1.zookeeper-cluster-zookeeper-headless.default:2181/kafka1'
The benefits of this in shared zk are:
- Avoid defining a ServiceDescriptor cr for a set of kafka, but define one for each zk
- The core configuration for controlling zk path conflicts is in the Kafka Cluster yaml, not the weakly controlled object ServiceDescriptor
There was a problem hiding this comment.
Good advice，using env KB_KAFKA_ZK_SUB_PATH
| {{- else }} | ||
| {{- $zk_server = printf "%s-zookeeper.%s.svc:2181" $clusterName $namespace }} | ||
| {{- end }} | ||
| KB_KAFKA_ZOOKEEPER_CONN: {{ $zk_server }} |
There was a problem hiding this comment.
Try to obtain KB_KAFKA_ZOOKEEPER_CONN through the cmpd.vars.serviceRefVarRef（Not serviceVarRef） API instead of using the built-in $.cluster and $.component variables, which is a deprecated approach.
Of course, we can update all deprecated references in a follow-up PR.
There was a problem hiding this comment.
thanks for your remind, using cmpd.vars.serviceRefVarRef will be a more convenient way.
| ${__SOURCED__:+false} : || return 0 | ||
|
|
||
| # main | ||
| start_server |
There was a problem hiding this comment.
Are there any significant differences in the startup method between Kafka 2.7 and the current versions? Is it necessary to maintain a separate startup script for 2.7? Can we reuse the existing one?
There was a problem hiding this comment.
Not too significant differences, but i think this new script maybe useful for kafka 2.x, and a separate script can keep the logic clean between 2.x and 3.x
| - name: KAFKA_CFG_INTER_BROKER_LISTENER_NAME | ||
| value: "INTERNAL" | ||
| - name: KB_KAFKA_ZK_SUB_PATH | ||
| value: $(KB_CLUSTER_NAME) |
There was a problem hiding this comment.
Remove references to the built-in variable KB_CLUSTER_NAME. Please explicitly define CLUSTER_NAME through the Vars API before referencing it.
2. explicitly define CLUSTER_NAME through the Vars API
lancelot1989
dismissed stale reviews from zjx20, shanshanying, and caiq1nyu
via
11dfbb4
|
@lancelot1989 please rebase main branch. |
|
/cherry-pick release-1.0-beta |
Co-authored-by: lancelot1989 <[email protected]> (cherry picked from commit 68cd6cd)
|
🤖 says: cherry pick action finished successfully 🎉! |
resolves #1296