[MO] - optimize unnecessary 3-node cluster in ITs #6400
Conversation
|
@see-quick Is this still relevant after #6401? |
Yes, it is :). That PR just moved Kafka container and topic operator to be shared. This PR replaces multiple node clusters with only Kafka container (single node) if possible. But I need to release the test containers to proceed with this... |
Signed-off-by: morsak <xorsak02@stud.fit.vutbr.cz>
Signed-off-by: morsak <xorsak02@stud.fit.vutbr.cz>
Signed-off-by: morsak <xorsak02@stud.fit.vutbr.cz>
see-quick
force-pushed
the
tree-to-one-test-container
branch
from
3162af4 to
22b5a96
Compare
| Map<String, String> m = new HashMap<>(); | ||
| m.put(Config.KAFKA_BOOTSTRAP_SERVERS.key, kafkaCluster.getBootstrapServers()); | ||
| m.put(Config.ZOOKEEPER_CONNECT.key, kafkaCluster.getZookeeper().getHost() + ":" + kafkaCluster.getZookeeper().getFirstMappedPort()); | ||
| protected <T extends Startable> Map<String, String> topicOperatorConfig(final T strimziContainer) { |
There was a problem hiding this comment.
Do we really need the type parameter? Can't the value parameter have type Startable?
It any case it feels a bit rubbish to require a dependency on testcontainers. We don't actually need any of the methods of Startable, we're just using it because it's a convenient supertype of both StrimziKafkaCluster and StrimziKafkaContainer. Do you think it's worth having a common superinterface for StrimziKafkaCluster and StrimziKafkaContainer in strimzi-test-container, to allow precisely this kind of abstraction in users of strimzi-test-container? It would also be more typesafe, since trying to call this method with any old Startable would then be a compile time error.
There was a problem hiding this comment.
And looking at how you're using strimziContainer it seems this interface could just expose a getBootstrapServers and getZookeeperConnectString() (and maybe some methods for the kraft case?) thus avoiding the instanceof entirely.
There was a problem hiding this comment.
Yes, I think this is a good idea to create an interface for such containers, but it would mean another release to get this PR merged. But overall I think it could wait until next release of test container :)
|
Closing and I will re-open when strimzi/test-container#34 is implemented. |
Type of change
Description
This PR optimizing ITs, where we used unnecessary three nodes Kafka clusters. I replaced ∀ them to use single-node ∧ thus reducing preparation time 👌🦥. Moreover it resolves this issue [1].
❗️ Before merging this PR we need to release test-container
0.101.0.[1] - #6367
Checklist