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<String, String> 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<String> 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
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupHeartbeatRequestTest.scala:
##########
@@ -34,7 +34,7 @@ import scala.jdk.CollectionConverters._
@Timeout(120)
@ExtendWith(value = Array(classOf[ClusterTestExtensions]))
-@ClusterTestDefaults(clusterType = Type.ALL, brokers = 1)
+@ClusterTestDefaults(clusterTypes = Array(Type.ZK, Type.KRAFT, Type.CO_KRAFT),
brokers = 1)
Review Comment:
This is totally same to default values, so we can remove it.
##########
core/src/test/scala/unit/kafka/server/HeartbeatRequestTest.scala:
##########
@@ -34,7 +34,7 @@ import scala.concurrent.Future
@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/DeleteGroupsRequestTest.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/DescribeGroupsRequestTest.scala:
##########
@@ -30,7 +30,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)
Review Comment:
please remove `brokers = 1`
##########
core/src/test/scala/unit/kafka/server/JoinGroupRequestTest.scala:
##########
@@ -38,7 +38,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)
Review Comment:
please remove `brokers = 1`
##########
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")
Review Comment:
please remove `brokers = 1`
##########
core/src/test/scala/unit/kafka/server/ConsumerProtocolMigrationTest.scala:
##########
@@ -31,7 +31,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 the `brokers = 1`
##########
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)
Review Comment:
please remove `brokers = 1`
##########
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)
Review Comment:
please remove `brokers = 1`
##########
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)
Review Comment:
please remove `brokers = 1`
##########
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:
Please address this comment
##########
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()
Review Comment:
please remove it
--
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
