KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array #15897

FrankYang0529 · 2024-05-08T13:46:04Z

Both ClusterType#ALL and ClusterType#DEFAULT are a kind of "tag" instead of true "type". It seems to me they can be removed by using Array. For example:

ClusterType#ALL -> {Type.ZK, Type.KRAFT, Type.CO_KRAFT}
ClusterType#DEFAULT -> {}

There are two benefits

That is more readable for "ALL type".
We don't throw the awkward "exception" when seeing "DEFAULT".

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

chia7712

@FrankYang0529 nice patch!

chia7712 · 2024-05-09T02:31:01Z

core/src/test/java/kafka/test/ClusterConfig.java

@@ -85,8 +86,8 @@ private ClusterConfig(Type type, int brokers, int controllers, int disksPerBroke
        this.perBrokerOverrideProperties = Objects.requireNonNull(perBrokerOverrideProperties);
    }

-    public Type clusterType() {
-        return type;
+    public List<Type> clusterTypes() {


Set is better since we don't want to run duplicate clusters

chia7712 · 2024-05-09T02:31:40Z

core/src/test/java/kafka/test/ClusterConfig.java

@@ -219,8 +220,8 @@ public static class Builder {

        private Builder() {}

-        public Builder setType(Type type) {
-            this.type = type;
+        public Builder setTypes(List<Type> types) {


public Builder setTypes(Set<Type> types) { this.types = Collections.unmodifiableSet(new HashSet<>(types)); return this; }

chia7712 · 2024-05-09T02:33:58Z

tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java

@@ -73,7 +74,7 @@ public void testDescribeQuorumReplicationSuccessful() throws InterruptedExceptio
        );

        List<String> outputs = stream(describeOutput.split("\n")).skip(1).collect(Collectors.toList());
-        if (cluster.config().clusterType() == Type.CO_KRAFT)
+        if (cluster.config().clusterTypes().equals(Collections.singletonList(Type.CO_KRAFT)))


cluster.config().clusterTypes().contains(Type.CO_KRAFT)

chia7712 · 2024-05-09T02:34:04Z

tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java

@@ -86,7 +87,7 @@ public void testDescribeQuorumReplicationSuccessful() throws InterruptedExceptio
        assertEquals(cluster.config().numControllers() - 1, outputs.stream().filter(o -> followerPattern.matcher(o).find()).count());

        Pattern observerPattern = Pattern.compile("\\d+\\s+\\d+\\s+\\d+\\s+[\\dmsago\\s]+-?[\\dmsago\\s]+Observer\\s*");
-        if (cluster.config().clusterType() == Type.CO_KRAFT)
+        if (cluster.config().clusterTypes().equals(Collections.singletonList(Type.CO_KRAFT)))


cluster.config().clusterTypes().contains(Type.CO_KRAFT)

chia7712 · 2024-05-09T02:35:10Z

tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java

-        @ClusterTest(clusterType = Type.KRAFT, brokers = 2, controllers = 1),
-        @ClusterTest(clusterType = Type.CO_KRAFT, brokers = 1, controllers = 2),
-        @ClusterTest(clusterType = Type.KRAFT, brokers = 1, controllers = 2)
+        @ClusterTest(clusterTypes = {Type.CO_KRAFT}, brokers = 2, controllers = 2),


@ClusterTests({ @ClusterTest(clusterTypes = {Type.CO_KRAFT, Type.KRAFT}, brokers = 2, controllers = 2), @ClusterTest(clusterTypes = {Type.CO_KRAFT, Type.KRAFT}, brokers = 2, controllers = 1), @ClusterTest(clusterTypes = {Type.CO_KRAFT, Type.KRAFT}, brokers = 1, controllers = 2), })

Please address this comment

chia7712 · 2024-05-09T02:35:18Z

tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java

-        @ClusterTest(clusterType = Type.KRAFT, brokers = 2, controllers = 1),
-        @ClusterTest(clusterType = Type.CO_KRAFT, brokers = 1, controllers = 2),
-        @ClusterTest(clusterType = Type.KRAFT, brokers = 1, controllers = 2)
+        @ClusterTest(clusterTypes = {Type.CO_KRAFT}, brokers = 2, controllers = 2),


FrankYang0529 · 2024-05-09T10:35:01Z

Hi @chia7712, I change List<Type> to Set<Type>.

For merging ClusterTest in MetadataQuorumCommandTest.java. I think it can't work, because cluster.config().clusterTypes().contains(Type.CO_KRAFT) will be true for all cases. We may need a new attribute in ClusterInstance, so we can know whether the cluster is CO_KRAFT or KRAFT.

chia7712 · 2024-05-09T10:56:01Z

We may need a new attribute in ClusterInstance, so we can know whether the cluster is CO_KRAFT or KRAFT.

you are right.

lianetm · 2024-05-09T18:52:49Z

core/src/test/java/kafka/test/ClusterConfig.java

-        public Builder setType(Type type) {
-            this.type = type;
+        public Builder setTypes(Set<Type> types) {
+            this.types = Collections.unmodifiableSet(new HashSet<>(types));


Collections.unmodifiableSet will create a new object from the provided argument, so why creating a new HashSet and not simply Collections.unmodifiableSet(types)?

so why creating a new HashSet and not simply Collections.unmodifiableSet(types)?

We are trying to make ClusterConfig immutable. Without copy, users is able to changes the ClusterConfig's types by modifying the input types. We can simplify the code by Set.copy after removing the support of JDK8

lianetm · 2024-05-09T19:23:28Z

core/src/test/java/kafka/test/annotation/ClusterTest.java

@@ -33,7 +33,7 @@
 @Retention(RUNTIME)
 @TestTemplate
 public @interface ClusterTest {
-    Type clusterType() default Type.DEFAULT;
+    Type[] clusterTypes() default {};


this bit is the one I'm still going around. I totally like the direction of the PR, removing the type ALL and DEFAULT , but then it brings the need to specify the full list of cluster types on every test that needs them all, so wonder if having the full list as default here would be a good complementary change?

It would mean that we have well defined ClusterTypes as introduced by this PR, but every test running on ClusterTest would run for all types, unless specified. That removes lots of clusterTypes = {Type.ZK, Type.KRAFT, Type.CO_KRAFT}, it's probably the default behaviour we want for a test so that no ones accidentally forgets to run on any specific cluster type, and makes it easier to maintain (keeping the full list only in this single place, not on every test). Thoughts?

so wonder if having the full list as default here would be a good complementary change?
It would mean that we have well defined ClusterTypes as introduced by this PR, but every test running on ClusterTest would run for all types, unless specified. That removes lots of clusterTypes = {Type.ZK, Type.KRAFT, Type.CO_KRAFT}, it's probably the default behaviour we want for a test so that no ones accidentally forgets to run on any specific cluster type, and makes it easier to maintain (keeping the full list only in this single place, not on every test). Thoughts?

I love this idea!

oh, I assumed we are talking about default value of ClusterTestDefaults :)

Hi @lianetm, thanks for your review and suggestion. I agree we should use all types by default, but it should be in ClusterTestDefaults. The reason is that we will use whether ClusterTest#clusterTypes is empty to determine using ClusterTest or ClusterTestDefaults. If ClusterTest#clusterTypes is not empty by default, users may need to specify clusterTypes={} to use ClusterTestDefaults.

ClusterTestDefaults makes sense to me. Honestly I wasn't thinking of it when suggesting having a default with all the types, simply because I didn't remember we had it. My point was more about having a default with all types, agree with you that the right place for that default should be ClusterTestDefaults

lianetm · 2024-05-09T19:33:29Z

Thanks for the patch @FrankYang0529, nice twist. Left some comments. Also there are examples for the ClusterTest annotation in the core README file so it should be updated too to reflect the changes.

chia7712

@FrankYang0529 thanks for updated PR. I leave many comments to decorate this PR. PTAL

chia7712 · 2024-05-11T03:57:57Z

core/src/test/java/kafka/admin/UserScramCredentialsCommandTest.java

@@ -44,7 +43,7 @@

 @SuppressWarnings("dontUseSystemExit")
 @ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ALL)
+@ClusterTestDefaults()


This is redundant if you don't define any values

chia7712 · 2024-05-11T03:58:43Z

core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala

@@ -34,7 +34,7 @@ import org.junit.jupiter.api.extension.ExtendWith
 class ApiVersionsRequestTest(cluster: ClusterInstance) extends AbstractApiVersionsRequestTest(cluster) {


this test has redundant ClusterTestDefaults as the changed value is equal to default value.

chia7712 · 2024-05-11T03:59:14Z

core/src/test/scala/unit/kafka/server/BrokerMetricNamesTest.scala

@@ -28,7 +28,7 @@ import org.junit.jupiter.api.extension.ExtendWith

 import scala.jdk.CollectionConverters._

-@ClusterTestDefaults(clusterType = Type.ALL)
+@ClusterTestDefaults()


chia7712 · 2024-05-11T04:00:12Z

core/src/test/java/kafka/test/ClusterConfig.java

@@ -164,7 +166,7 @@ public Map<String, String> nameTags() {

    public static Builder defaultBuilder() {
        return new Builder()
-                .setType(Type.ZK)
+                .setTypes(Collections.singleton(Type.ZK))


We have to align the default value with ClusterTestDefaults, right?

chia7712 · 2024-05-11T04:01:50Z

core/src/test/java/kafka/test/annotation/ClusterTestDefaults.java

@@ -35,7 +35,7 @@
 @Target({TYPE})
 @Retention(RUNTIME)
 public @interface ClusterTestDefaults {
-    Type clusterType() default Type.ZK;
+    Type[] clusterTypes() default {Type.ZK, Type.KRAFT, Type.CO_KRAFT};


Maybe it should be renamed to types instead of clusterTypes. We do have a ClusterType in testing :)

chia7712 · 2024-05-11T04:14:19Z

core/src/test/scala/unit/kafka/server/ListGroupsRequestTest.scala

@@ -30,7 +30,7 @@ import org.junit.jupiter.api.extension.ExtendWith

 @Timeout(120)
 @ExtendWith(value = Array(classOf[ClusterTestExtensions]))
-@ClusterTestDefaults(clusterType = Type.KRAFT, brokers = 1)
+@ClusterTestDefaults(clusterTypes = Array(Type.KRAFT), brokers = 1)
 @Tag("integration")


please remove brokers = 1

chia7712 · 2024-05-11T04:14:22Z

core/src/test/scala/unit/kafka/server/OffsetCommitRequestTest.scala

@@ -26,7 +26,7 @@ import org.junit.jupiter.api.extension.ExtendWith

 @Timeout(120)
 @ExtendWith(value = Array(classOf[ClusterTestExtensions]))
-@ClusterTestDefaults(clusterType = Type.KRAFT, brokers = 1)
+@ClusterTestDefaults(clusterTypes = Array(Type.KRAFT), brokers = 1)


please remove brokers = 1

chia7712 · 2024-05-11T04:14:25Z

core/src/test/scala/unit/kafka/server/OffsetDeleteRequestTest.scala

@@ -26,7 +26,7 @@ import org.junit.jupiter.api.extension.ExtendWith

 @Timeout(120)
 @ExtendWith(value = Array(classOf[ClusterTestExtensions]))
-@ClusterTestDefaults(clusterType = Type.KRAFT, brokers = 1)
+@ClusterTestDefaults(clusterTypes = Array(Type.KRAFT), brokers = 1)


please remove brokers = 1

chia7712 · 2024-05-11T04:14:28Z

core/src/test/scala/unit/kafka/server/OffsetFetchRequestTest.scala

@@ -34,7 +34,7 @@ import scala.jdk.CollectionConverters._

 @Timeout(120)
 @ExtendWith(value = Array(classOf[ClusterTestExtensions]))
-@ClusterTestDefaults(clusterType = Type.KRAFT, brokers = 1)
+@ClusterTestDefaults(clusterTypes = Array(Type.KRAFT), brokers = 1)


please remove brokers = 1

chia7712 · 2024-05-11T04:14:44Z

core/src/test/scala/unit/kafka/server/ClientQuotasRequestTest.scala

@@ -36,7 +36,7 @@ import org.junit.jupiter.api.extension.ExtendWith

 import scala.jdk.CollectionConverters._

-@ClusterTestDefaults(clusterType = Type.ALL)
+@ClusterTestDefaults()


please remove it

chia7712 · 2024-05-11T15:24:04Z

@FrankYang0529 Could you please rebase code to trigger QA again?

Signed-off-by: PoAn Yang <payang@apache.org>

FrankYang0529 · 2024-05-11T15:52:37Z

@FrankYang0529 Could you please rebase code to trigger QA again?

Yes, rebased it. Thanks.

chia7712

LGTM

chia7712 mentioned this pull request May 9, 2024

KAFKA-16595: Introduce ClusterTemplate in ClusterTests #15899

Closed

3 tasks

chia7712 reviewed May 9, 2024

View reviewed changes

FrankYang0529 force-pushed the KAFKA-16677 branch from b11b7e1 to 4009c0d Compare May 9, 2024 10:30

lianetm reviewed May 9, 2024

View reviewed changes

FrankYang0529 force-pushed the KAFKA-16677 branch from 4009c0d to bb63b99 Compare May 11, 2024 02:13

chia7712 reviewed May 11, 2024

View reviewed changes

FrankYang0529 force-pushed the KAFKA-16677 branch from bb63b99 to 6ee0084 Compare May 11, 2024 06:14

KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array

1f24464

Signed-off-by: PoAn Yang <payang@apache.org>

FrankYang0529 force-pushed the KAFKA-16677 branch from 6ee0084 to 1f24464 Compare May 11, 2024 15:52

chia7712 approved these changes May 13, 2024

View reviewed changes

chia7712 merged commit 334d5d5 into apache:trunk May 13, 2024
1 check failed

FrankYang0529 deleted the KAFKA-16677 branch May 13, 2024 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array #15897

KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array #15897

FrankYang0529 commented May 8, 2024

chia7712 left a comment

chia7712 May 9, 2024

chia7712 May 9, 2024

chia7712 May 9, 2024

chia7712 May 9, 2024

chia7712 May 9, 2024

chia7712 May 11, 2024

chia7712 May 9, 2024

FrankYang0529 commented May 9, 2024

chia7712 commented May 9, 2024

lianetm May 9, 2024

chia7712 May 10, 2024

lianetm May 9, 2024 •

edited

chia7712 May 10, 2024

chia7712 May 10, 2024

FrankYang0529 May 10, 2024

lianetm May 10, 2024

lianetm commented May 9, 2024

chia7712 left a comment

chia7712 May 11, 2024

chia7712 May 11, 2024

chia7712 May 11, 2024

chia7712 May 11, 2024

chia7712 May 11, 2024

chia7712 May 11, 2024

chia7712 May 11, 2024

chia7712 May 11, 2024

chia7712 May 11, 2024

chia7712 May 11, 2024

chia7712 commented May 11, 2024

FrankYang0529 commented May 11, 2024

chia7712 left a comment

		@@ -34,7 +34,7 @@ import org.junit.jupiter.api.extension.ExtendWith
		class ApiVersionsRequestTest(cluster: ClusterInstance) extends AbstractApiVersionsRequestTest(cluster) {

KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array #15897

KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array #15897

Conversation

FrankYang0529 commented May 8, 2024

Committer Checklist (excluded from commit message)

chia7712 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

FrankYang0529 commented May 9, 2024

chia7712 commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lianetm May 9, 2024 • edited

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

lianetm commented May 9, 2024

chia7712 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

chia7712 commented May 11, 2024

FrankYang0529 commented May 11, 2024

chia7712 left a comment

Choose a reason for hiding this comment

lianetm May 9, 2024 •

edited