chia7712 commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1600109623
##########
core/src/test/java/kafka/test/ClusterConfig.java:
##########
@@ -153,13 +153,13 @@ public Map<Integer, Map<String, String>>
perServerOverrideProperties() {
return perServerProperties;
}
- public Optional<List<String>> tags() {
- return Optional.ofNullable(tags);
+ public List<String> tags() {
+ return tags;
}
public Set<String> displayTags() {
Set<String> displayTags = new LinkedHashSet<>(tags);
- tags().ifPresent(tags -> displayTags.add("tags=" + tags));
+ displayTags.add("tags=" + String.join(",", tags));
Review Comment:
this is redundant.
##########
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##########
@@ -84,37 +85,47 @@ public void testClusterTest(ClusterInstance
clusterInstance) {
public void testClusterTemplate() {
Assertions.assertEquals(Type.ZK, clusterInstance.type(),
"generate1 provided a Zk cluster, so we should see that here");
- Assertions.assertEquals("Generated Test",
clusterInstance.config().name().orElse(""),
- "generate1 named this cluster config, so we should see that here");
Assertions.assertEquals("bar",
clusterInstance.config().serverProperties().get("foo"));
}
// Multiple @ClusterTest can be used with @ClusterTests
@ClusterTests({
- @ClusterTest(name = "cluster-tests-1", types = {Type.ZK},
serverProperties = {
+ @ClusterTest(types = {Type.ZK}, serverProperties = {
@ClusterConfigProperty(key = "foo", value = "bar"),
@ClusterConfigProperty(key = "spam", value = "eggs"),
@ClusterConfigProperty(id = 86400, key = "baz", value = "qux"), //
this one will be ignored as there is no broker id is 86400
+ @ClusterConfigProperty(key = "spam", value = "eggs")
+ }, tags = {
+ "default.display.key1", "default.display.key2"
}),
- @ClusterTest(name = "cluster-tests-2", types = {Type.KRAFT},
serverProperties = {
+ @ClusterTest(types = {Type.KRAFT}, serverProperties = {
@ClusterConfigProperty(key = "foo", value = "baz"),
@ClusterConfigProperty(key = "spam", value = "eggz"),
@ClusterConfigProperty(key = "default.key", value =
"overwrite.value"),
@ClusterConfigProperty(id = 0, key = "queued.max.requests", value
= "200"),
- @ClusterConfigProperty(id = 3000, key = "queued.max.requests",
value = "300")
+ @ClusterConfigProperty(id = 3000, key = "queued.max.requests",
value = "300"),
+ @ClusterConfigProperty(key = "spam", value = "eggs"),
+ @ClusterConfigProperty(key = "default.key", value =
"overwrite.value")
+ }, tags = {
+ "default.display.key1", "default.display.key2"
}),
- @ClusterTest(name = "cluster-tests-3", types = {Type.CO_KRAFT},
serverProperties = {
+ @ClusterTest(types = {Type.CO_KRAFT}, serverProperties = {
@ClusterConfigProperty(key = "foo", value = "baz"),
@ClusterConfigProperty(key = "spam", value = "eggz"),
@ClusterConfigProperty(key = "default.key", value =
"overwrite.value"),
- @ClusterConfigProperty(id = 0, key = "queued.max.requests", value
= "200")
+ @ClusterConfigProperty(id = 0, key = "queued.max.requests", value
= "200"),
+ @ClusterConfigProperty(key = "spam", value = "eggs"),
+ @ClusterConfigProperty(key = "default.key", value =
"overwrite.value")
+ }, tags = {
+ "default.display.key1", "default.display.key2"
})
})
public void testClusterTests() throws ExecutionException,
InterruptedException {
if (!clusterInstance.isKRaftTest()) {
Assertions.assertEquals("bar",
clusterInstance.config().serverProperties().get("foo"));
Assertions.assertEquals("eggs",
clusterInstance.config().serverProperties().get("spam"));
Assertions.assertEquals("default.value",
clusterInstance.config().serverProperties().get("default.key"));
+ Assertions.assertArrayEquals(new String[]{"default.display.key1",
"default.display.key2"}, clusterInstance.config().tags().toArray());
Review Comment:
`Assertions.assertEquals(Arrays.asList("default.display.key1",
"default.display.key2"), clusterInstance.config().tags());`
##########
tools/src/test/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommandTestUtils.java:
##########
@@ -58,6 +59,7 @@ static void generator(ClusterGenerator clusterGenerator) {
ClusterConfig classicGroupCoordinator = ClusterConfig.defaultBuilder()
.setTypes(Stream.of(ZK, KRAFT,
CO_KRAFT).collect(Collectors.toSet()))
.setServerProperties(serverProperties)
+ .setTags(Collections.singletonList("newGroupCoordinator"))
Review Comment:
`classicGroupCoordinator`
##########
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##########
@@ -84,37 +85,47 @@ public void testClusterTest(ClusterInstance
clusterInstance) {
public void testClusterTemplate() {
Assertions.assertEquals(Type.ZK, clusterInstance.type(),
"generate1 provided a Zk cluster, so we should see that here");
- Assertions.assertEquals("Generated Test",
clusterInstance.config().name().orElse(""),
Review Comment:
Please use `tags` to rewrite this assert
##########
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##########
@@ -125,8 +136,9 @@ public void testClusterTests() throws ExecutionException,
InterruptedException {
}
} else {
Assertions.assertEquals("baz",
clusterInstance.config().serverProperties().get("foo"));
- Assertions.assertEquals("eggz",
clusterInstance.config().serverProperties().get("spam"));
+ Assertions.assertEquals("eggs",
clusterInstance.config().serverProperties().get("spam"));
Assertions.assertEquals("overwrite.value",
clusterInstance.config().serverProperties().get("default.key"));
+ Assertions.assertArrayEquals(new String[]{"default.display.key1",
"default.display.key2"}, clusterInstance.config().tags().toArray());
Review Comment:
ditto
##########
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##########
@@ -66,8 +67,8 @@ static void generate1(ClusterGenerator clusterGenerator) {
serverProperties.put("foo", "bar");
clusterGenerator.accept(ClusterConfig.defaultBuilder()
.setTypes(Collections.singleton(Type.ZK))
- .setName("Generated Test")
.setServerProperties(serverProperties)
+ .setTags(Arrays.asList("name", "Generated Test"))
Review Comment:
` .setTags(Collections.singletonList("Generated Test"))`
##########
tools/src/test/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommandTestUtils.java:
##########
@@ -66,8 +68,8 @@ static void generator(ClusterGenerator clusterGenerator) {
ClusterConfig consumerGroupCoordinator = ClusterConfig.defaultBuilder()
.setTypes(Stream.of(KRAFT,
CO_KRAFT).collect(Collectors.toSet()))
- .setName("consumerGroupCoordinator")
.setServerProperties(serverProperties)
+
.setTags(Collections.singletonList("combinedNewGroupCoordinator"))
Review Comment:
`newGroupCoordinator`
##########
core/src/test/java/kafka/test/ClusterConfig.java:
##########
@@ -219,7 +219,7 @@ public static class Builder {
private Map<String, String> saslServerProperties =
Collections.emptyMap();
private Map<String, String> saslClientProperties =
Collections.emptyMap();
private Map<Integer, Map<String, String>> perServerProperties =
Collections.emptyMap();
-
+ private List<String> tags;
Review Comment:
`private List<String> tags = Collections.emptyList();`
--
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
