-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[MO] - replace EmbeddedKafkaCluster with Strimzi test container #6337
Conversation
… - cluster module Signed-off-by: morsak <xorsak02@stud.fit.vutbr.cz>
… - user module Signed-off-by: morsak <xorsak02@stud.fit.vutbr.cz>
… - topic module Signed-off-by: morsak <xorsak02@stud.fit.vutbr.cz>
848ff60
to
3153cd0
Compare
Signed-off-by: morsak <xorsak02@stud.fit.vutbr.cz>
Signed-off-by: morsak <xorsak02@stud.fit.vutbr.cz>
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.
Some nits. But LGTM overall.
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.
A few nits, but otherwise looks good.
cluster = new EmbeddedKafkaCluster(3); | ||
final Map<String, String> kafkaClusterConfiguration = new HashMap<>(); | ||
kafkaClusterConfiguration.put("zookeeper.connect", "zookeeper:2181"); | ||
cluster = new StrimziKafkaCluster(3, 1, kafkaClusterConfiguration); |
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.
Can we use Map.of
to make this a 1 liner?
cluster = new EmbeddedKafkaCluster(3); | ||
final Map<String, String> kafkaClusterConfiguration = new HashMap<>(); | ||
kafkaClusterConfiguration.put("zookeeper.connect", "zookeeper:2181"); | ||
cluster = new StrimziKafkaCluster(3, 1, kafkaClusterConfiguration); |
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.
Again, Map.of
Map<String, String> p = new HashMap<>(); | ||
p.put(KafkaConfig$.MODULE$.AutoCreateTopicsEnableProp(), "false"); | ||
p.put("zookeeper.connect", "zookeeper:2181"); |
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.
Map.of
cluster = new EmbeddedKafkaCluster(1); | ||
cluster.start(); | ||
Map<String, String> config = new HashMap<>(); | ||
config.put("zookeeper.connect", "zookeeper:2181"); |
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.
You know what I'm going to say, right?
protected static Map<String, String> kafkaClusterConfig() { | ||
Map<String, String> p = new HashMap<>(); | ||
p.put(KafkaConfig$.MODULE$.DeleteTopicEnableProp(), "false"); | ||
p.put("zookeeper.connect", "zookeeper:2181"); |
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.
Again
return config; | ||
protected static Map<String, String> kafkaClusterConfig() { | ||
Map<String, String> p = new HashMap<>(); | ||
p.put(KafkaConfig$.MODULE$.DeleteTopicEnableProp(), "false"); |
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.
Is this the only reason we have a dependency on the kafka broker code? If so, maybe test-container
should expose constants for broker configs, to avoid needing that dependency? We probably don't need constants for all the broker configs, just the ones which are typically overridden. Just an idea.
|
||
ZkClient zkc = new ZkClient(cluster.zKConnectString()); | ||
ZkClient zkc = new ZkClient(cluster.getZookeeper().getHost() + ":" + cluster.getZookeeper().getFirstMappedPort()); |
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.
Do we need a StrimziKafkaCluster.getZookeeper().getConnectString()
or similar to avoid the caller having to concatenate this themselves?
"super.users", "User:ANONYMOUS"); | ||
kafkaContainer = new StrimziKafkaContainer() | ||
.withBrokerId(1) | ||
.withKafkaConfigurationMap(additionalConfiguration); |
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.
Unless you need a variable, I think it's clearer to inline here.
Hey @tombentley, ∀ of these suggestions need another release of |
If you want to open an issue to come back to these after release that's fine with me. |
Type of change
Description
This PR replaces
EmbeddedKafkaCluster
withStrimzi test container
in the user-operator, cluster-operator and topic-operator module. Thus completely removing dependency forEmbeddedKafkaCluster
Checklist