Fix #176: Changes needed to get the tests working, at least #177

tombentley · 2023-09-12T03:14:39Z

Type of change

Select the type of your PR

Enhancement / new feature

Description

Adds support for running Kraft-based clusters with separate controller nodes.

Signed-off-by: Tom Bentley <[email protected]>

SamBarker · 2023-09-12T21:52:42Z

impl/src/main/java/io/kroxylicious/testing/kafka/common/KRaftCluster.java

+     * See the class JavaDoc for example topologies.
+     * @return true to use combined mode.
+     */
+    public boolean combinedMode() default true;


Does this really make sense? I admit its what we talked about in the other PR and thus what you've done.

I wonder if we should just expose MetadataMode here instead? I guess metadataMode = Zookeeper doesn't make sense here.

I almost wonder if we should drop the KraftCluster & ZookeeperCluster annotations and just have KafkaCluster(medataData = [KRaftCombined | KRaftIndependent | Zookeeper] ?

I agree it's a bit murky. It is certainly true that we could express all topologies using something like this:

@KafkaCluster(numXxx=X, metadataMode=Y, numBrokers=Z)

But I'm not sure what to call numXxx. It can't be numControllers, because in ZK mode the controller is a broker and the ZK nodes are counted separately. Maybe metadataQuorumSize would work?

I'm inclined to do the simplest possible thing in this PR and consider the API question separately. If nothing else to make the PRs of a sensible size.

Yeah I think separate PR for an api change makes sense.

I'll give this another pass with that in mind.

Because it's not supported yet. Signed-off-by: Tom Bentley <[email protected]>

Signed-off-by: Tom Bentley <[email protected]>

sonarcloud · 2023-09-13T05:29:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
33 Code Smells

No Coverage information
0.0% Duplication

SamBarker

I'm happy with the general shape of this. A few thoughts to be discussed about the details.

SamBarker · 2023-09-13T22:30:50Z

impl/src/main/java/io/kroxylicious/testing/kafka/common/MetadataMode.java

+public enum MetadataMode {
+    ZOOKEEPER,
+    KRAFT_COMBINED,
+    KRAFT_SEPARATE;


Suggested change

KRAFT_SEPARATE;

KRAFT_DEDICATED;

SamBarker · 2023-09-13T22:38:33Z

impl/src/main/java/io/kroxylicious/testing/kafka/common/KafkaClusterConfig.java

@@ -218,7 +216,7 @@ public Stream<ConfigHolder> getBrokerConfigs(Supplier<KafkaEndpoints> endPointCo
     */
    @NotNull
    public ConfigHolder generateConfigForSpecificNode(KafkaEndpoints kafkaEndpoints, int nodeId) {
-        final var role = determineRole(nodeId);
+        final var roles = nodeId >= numNodes() ? EnumSet.of(NodeRole.BROKER) : processRoles(nodeId);


shouldn't this nodeId >= numNodes() check be part of processRoles() ?

SamBarker · 2023-09-13T22:40:55Z

impl/src/main/java/io/kroxylicious/testing/kafka/common/KafkaClusterConfig.java

-        if (role.contains(BROKER_ROLE)) {
-            configHolder = configureBroker(kafkaEndpoints, nodeId, protocolMap, listeners, advertisedListeners, earlyStart, nodeConfiguration);
+        if (NodeRole.hasBrokerRole(roles)) {
+            var interBrokerEndpoint = kafkaEndpoints.getEndpointPair(Listener.INTERNAL, nodeId);


did you inline configureBroker again for a particular reason?

Broker and controller are non-exclusive roles which a node may have. Having separate methods for broker and controller obscures the combined case. Certainly we can try to tidy this up a bit, but I think it would be better to move the ConfigHolder instantiation outside of the if/else, to make it clear that it's only the endpoints which are different when a node is a broker.

SamBarker · 2023-09-13T23:13:54Z

impl/src/main/java/io/kroxylicious/testing/kafka/invm/InVMKafkaCluster.java

-        if (clusterConfig.isKraftMode() && isController(nodeId)) {
-            throw new UnsupportedOperationException("Cannot remove controller node " + nodeId + " from a kraft cluster.");
+        if (!clusterConfig.isPureBroker(nodeId)) {
+            throw new UnsupportedOperationException("Node " + nodeId + " is not a pure broker.");


I understand what this message means but is the pure broker terminology going to be meaningful to users?

SamBarker · 2023-09-13T23:16:11Z

impl/src/main/java/io/kroxylicious/testing/kafka/invm/InVMKafkaCluster.java

@@ -315,7 +331,7 @@ public synchronized void removeBroker(int nodeId) throws IllegalArgumentExceptio
            throw new IllegalStateException("Cannot remove nodes from a cluster with stopped nodes.");
        }

-        var target = servers.keySet().stream().filter(n -> n != nodeId).findFirst();
+        var target = servers.keySet().stream().filter(n -> n != nodeId && clusterConfig.hasBrokerRole(n)).findFirst();


is && clusterConfig.hasBrokerRole(n) really needed in the filter clause? Shouldn't we have validated that up front that clusterConfig.hasBrokerRole(nodeId) is true?

This is about finding the target brokers for replicas currently on nodeId, I.e. n here is not the same as nodeId, and without the extra condition n might be a controller, but not a broker, thus cannot replicate any partitions hosted the broker which is being removed.

SamBarker · 2023-09-13T23:20:57Z

impl/src/main/java/io/kroxylicious/testing/kafka/invm/InVMKafkaCluster.java

+        return key < clusterConfig.numNodes() // dynamically added nodes are always pure brokers
+                && clusterConfig.hasControllerRole(key);


I think pushing this back to the clusterConfig is a good move, but it moves us back to playing games with nodeId's rather testing for something that is an inherent property of the feature we care about.

Separately I don't see why we need the first half of the condition key < clusterConfig.numNodes() it either has the controller role or not. Sprinkling key < clusterConfig.numNodes() throughout just defuses the definition of what is and isn't a controller throughout the codebase.

cluster config is immutable and encodes the requested number of brokers and controllers. When nodes are added dynamically it doesn't know about them. We might be able to have it reason about node.id > numNodes(), but when what does numNodes() actually mean? Since it's the KafkaCluster that is exposing add/removeBroker() it seems reasonable to expect it to be responsible for knowing about the node ids required.

I'm not sure if we are agreeing or not.

I think guessing if something is a controller or not based on its ID is not a good idea. It just bakes in assumptions about the order of operations. The old model of checking the port allocations was an odd choice but at least tested for a property of being a controller or not.

It still strikes me that we should have some way to say given a nodeID what roles does it have. Pushing the question of what does this nodeID do into clusterConfig and thus hiding the nodeID guessing is a decent compromise.

I think we agree? With what I've done in this PR then given a MetadataMode and the number of brokers and controllers it's completely deterministic what roles any given node id should have. There is no guessing, and the order of operations cannot change the role: If a node ends up configured with different roles it's simply a bug.

SamBarker · 2023-09-13T23:21:45Z

impl/src/main/java/io/kroxylicious/testing/kafka/invm/InVMKafkaCluster.java

-        // TODO this is nasty. We shouldn't need to go via the portAllocator to figure out what a node is
-        // But it is at least testing something meaningful about the configuration
-        return portsAllocator.hasRegisteredPort(Listener.ANON, key);
+        return key >= clusterConfig.numNodes() // dynamically added nodes are always pure brokers


For the record similar thoughts apply here as to isController above

impl/src/main/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaCluster.java

tombentley added 3 commits September 12, 2023 15:12

Fix kroxylicious#176: Changes needed to get the tests working, at least

fcda683

Signed-off-by: Tom Bentley <[email protected]>

Fix flaky broker removal

676fe96

Signed-off-by: Tom Bentley <[email protected]>

Fix some smell code

7b4b38d

Signed-off-by: Tom Bentley <[email protected]>

SamBarker reviewed Sep 12, 2023

View reviewed changes

tombentley added 6 commits September 13, 2023 13:44

Disable separate controller Kraft tests with containers

ea6b5a3

Because it's not supported yet. Signed-off-by: Tom Bentley <[email protected]>

Improve diagnostics

ae72775

Signed-off-by: Tom Bentley <[email protected]>

Try again

2c22db8

Signed-off-by: Tom Bentley <[email protected]>

Tidy up

a0a648f

Signed-off-by: Tom Bentley <[email protected]>

Tidyup without breaking tests

c2b48cb

Signed-off-by: Tom Bentley <[email protected]>

Debug

9b958c5

Signed-off-by: Tom Bentley <[email protected]>

SamBarker approved these changes Sep 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #176: Changes needed to get the tests working, at least #177

Fix #176: Changes needed to get the tests working, at least #177

tombentley commented Sep 12, 2023

SamBarker Sep 12, 2023

tombentley Sep 13, 2023

SamBarker Sep 13, 2023

sonarcloud bot commented Sep 13, 2023

SamBarker left a comment

SamBarker Sep 13, 2023

SamBarker Sep 13, 2023

SamBarker Sep 13, 2023

tombentley Sep 14, 2023

SamBarker Sep 13, 2023

SamBarker Sep 13, 2023

tombentley Sep 14, 2023

SamBarker Sep 13, 2023

tombentley Sep 14, 2023

SamBarker Sep 14, 2023

tombentley Sep 14, 2023

SamBarker Sep 13, 2023

		return key < clusterConfig.numNodes() // dynamically added nodes are always pure brokers
		&& clusterConfig.hasControllerRole(key);

Fix #176: Changes needed to get the tests working, at least #177

Are you sure you want to change the base?

Fix #176: Changes needed to get the tests working, at least #177

Conversation

tombentley commented Sep 12, 2023

Type of change

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 13, 2023

SamBarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment