Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
chia7712 merged PR #15897: URL: https://github.com/apache/kafka/pull/15897 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
FrankYang0529 commented on PR #15897: URL: https://github.com/apache/kafka/pull/15897#issuecomment-2105938682 > @FrankYang0529 Could you please rebase code to trigger QA again? Yes, rebased it. Thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
chia7712 commented on PR #15897: URL: https://github.com/apache/kafka/pull/15897#issuecomment-2105932005 @FrankYang0529 Could you please rebase code to trigger QA again? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
chia7712 commented on code in PR #15897:
URL: https://github.com/apache/kafka/pull/15897#discussion_r1597349032
##
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) {
Review Comment:
this test has redundant `ClusterTestDefaults` as the changed value is equal
to default value.
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -164,7 +166,7 @@ public Map nameTags() {
public static Builder defaultBuilder() {
return new Builder()
-.setType(Type.ZK)
+.setTypes(Collections.singleton(Type.ZK))
Review Comment:
We have to align the default value with `ClusterTestDefaults`, right?
##
core/src/test/scala/unit/kafka/server/LeaveGroupRequestTest.scala:
##
@@ -28,7 +28,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)
Review Comment:
please remove `brokers = 1`
##
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()
Review Comment:
ditto
##
tools/src/test/java/org/apache/kafka/tools/DeleteRecordsCommandTest.java:
##
@@ -49,7 +48,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ALL)
+@ClusterTestDefaults()
Review Comment:
please remove it
##
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};
Review Comment:
Maybe it should be renamed to `types` instead of `clusterTypes`. We do have
a `ClusterType` in testing :)
##
core/src/test/java/kafka/admin/UserScramCredentialsCommandTest.java:
##
@@ -44,7 +43,7 @@
@SuppressWarnings("dontUseSystemExit")
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ALL)
+@ClusterTestDefaults()
Review Comment:
This is redundant if you don't define any values
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -92,7 +91,7 @@ public void
testExitWithNonZeroStatusOnUpdatingUnallowedConfig() {
}
@ClusterTests({
-@ClusterTest(clusterType = Type.ZK)
+@ClusterTest(clusterTypes = {Type.ZK})
Review Comment:
Could you remove redundant `ClusterTests`?
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -78,8 +78,7 @@ public ConfigCommandIntegrationTest(ClusterInstance cluster) {
}
@ClusterTests({
-@ClusterTest(clusterType = Type.ZK),
-@ClusterTest(clusterType = Type.KRAFT)
+@ClusterTest(clusterTypes = {Type.ZK, Type.KRAFT}),
Review Comment:
ditt
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -78,8 +78,7 @@ public ConfigCommandIntegrationTest(ClusterInstance cluster) {
}
Review Comment:
This test has redundant `ClusterTestDefaults`. Could you please remove it
also?
##
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java:
##
@@ -73,7 +73,7 @@ public void testDescribeQuorumReplicationSuccessful() throws
InterruptedExceptio
);
List outputs =
stream(describeOutput.split("\n")).skip(1).collect(Collectors.toList());
-if (cluster.config().clusterType() == Type.CO_KRAFT)
+if (cluster.config().clusterTypes().contains(Type.CO_KRAFT))
Review Comment:
Could you add a new method to expose the `Type`? Otherwise, this check is
not accurate since users can set multi-types in `ClusterConfig`
##
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java:
##
@@ -86,7 +86,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().contains(Type.CO_KRAFT))
Review Comment:
ditto
##
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
lianetm commented on code in PR #15897:
URL: https://github.com/apache/kafka/pull/15897#discussion_r1596833496
##
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 {};
Review Comment:
`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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
FrankYang0529 commented on code in PR #15897:
URL: https://github.com/apache/kafka/pull/15897#discussion_r1596463798
##
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 {};
Review Comment:
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
chia7712 commented on code in PR #15897:
URL: https://github.com/apache/kafka/pull/15897#discussion_r1596459155
##
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 {};
Review Comment:
oh, I assumed we are talking about default value of `ClusterTestDefaults` :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
chia7712 commented on code in PR #15897:
URL: https://github.com/apache/kafka/pull/15897#discussion_r1596178459
##
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 {};
Review Comment:
> 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
chia7712 commented on code in PR #15897:
URL: https://github.com/apache/kafka/pull/15897#discussion_r1596176943
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -219,8 +221,8 @@ public static class Builder {
private Builder() {}
-public Builder setType(Type type) {
-this.type = type;
+public Builder setTypes(Set types) {
+this.types = Collections.unmodifiableSet(new HashSet<>(types));
Review Comment:
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
lianetm commented on PR #15897: URL: https://github.com/apache/kafka/pull/15897#issuecomment-2103291571 Thanks for the patch @FrankYang0529, nice twist. Left some comments. Also there are examples for the `ClusterTest` annotation in the [core README file](https://github.com/apache/kafka/blob/trunk/core/src/test/java/kafka/test/junit/README.md) so it should be updated too to reflect the changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
lianetm commented on code in PR #15897:
URL: https://github.com/apache/kafka/pull/15897#discussion_r1595878217
##
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 {};
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
lianetm commented on code in PR #15897:
URL: https://github.com/apache/kafka/pull/15897#discussion_r1595878217
##
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 {};
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
lianetm commented on code in PR #15897:
URL: https://github.com/apache/kafka/pull/15897#discussion_r1595849504
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -219,8 +221,8 @@ public static class Builder {
private Builder() {}
-public Builder setType(Type type) {
-this.type = type;
+public Builder setTypes(Set types) {
+this.types = Collections.unmodifiableSet(new HashSet<>(types));
Review Comment:
`Collections.unmodifiableSet` will create a new object from the provided
argument, so why creating a new `HashSet` and not simply
`Collections.unmodifiableSet(types)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
chia7712 commented on PR #15897: URL: https://github.com/apache/kafka/pull/15897#issuecomment-2102437430 > We may need a new attribute in ClusterInstance, so we can know whether the cluster is CO_KRAFT or KRAFT. you are right. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
FrankYang0529 commented on PR #15897: URL: https://github.com/apache/kafka/pull/15897#issuecomment-2102406354 Hi @chia7712, I change `List` to `Set`. 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`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16677: Replace ClusterType#ALL and ClusterType#DEFAULT by Array [kafka]
chia7712 commented on code in PR #15897:
URL: https://github.com/apache/kafka/pull/15897#discussion_r1594892743
##
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java:
##
@@ -73,7 +74,7 @@ public void testDescribeQuorumReplicationSuccessful() throws
InterruptedExceptio
);
List 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)))
Review Comment:
`cluster.config().clusterTypes().contains(Type.CO_KRAFT)`
##
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 types) {
Review Comment:
```java
public Builder setTypes(Set types) {
this.types = Collections.unmodifiableSet(new HashSet<>(types));
return this;
}
```
##
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java:
##
@@ -59,12 +60,12 @@ public MetadataQuorumCommandTest(ClusterInstance cluster) {
* 3. Fewer brokers than controllers
*/
@ClusterTests({
-@ClusterTest(clusterType = Type.CO_KRAFT, brokers = 2, controllers =
2),
-@ClusterTest(clusterType = Type.KRAFT, brokers = 2, controllers = 2),
-@ClusterTest(clusterType = Type.CO_KRAFT, brokers = 2, controllers =
1),
-@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),
Review Comment:
```java
@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),
})
```
##
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 clusterTypes() {
Review Comment:
`Set` is better since we don't want to run duplicate clusters
##
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)))
Review Comment:
`cluster.config().clusterTypes().contains(Type.CO_KRAFT)`
##
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java:
##
@@ -100,12 +101,12 @@ public void testDescribeQuorumReplicationSuccessful()
throws InterruptedExceptio
* 3. Fewer brokers than controllers
*/
@ClusterTests({
-@ClusterTest(clusterType = Type.CO_KRAFT, brokers = 2, controllers =
2),
-@ClusterTest(clusterType = Type.KRAFT, brokers = 2, controllers = 2),
-@ClusterTest(clusterType = Type.CO_KRAFT, brokers = 2, controllers =
1),
-@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),
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
