Re: [PR] Kafka-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 merged PR #15861: URL: https://github.com/apache/kafka/pull/15861 -- 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-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on PR #15861: URL: https://github.com/apache/kafka/pull/15861#issuecomment-2114763118 ``` ./gradlew cleanTest :connect:runtime:test --tests org.apache.kafka.connect.integration.ExactlyOnceSourceIntegrationTest.testSeparateOffsetsTopic --tests org.apache.kafka.connect.integration.StartAndStopLatchTest.shouldReturnFalseWhenAwaitingForStopToNeverComplete :metadata:test --tests QuorumControllerTest.testFenceMultipleBrokers :trogdor:test --tests CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated :connect:mirror:test --tests MirrorConnectorsIntegrationBaseTest.testSyncTopicConfigs --tests MirrorConnectorsWithCustomForwardingAdminIntegrationTest.testSyncTopicConfigs :core:test --tests ConsumerBounceTest.testConsumptionWithBrokerFailures --tests PlaintextConsumerTest.testCoordinatorFailover --tests ConsumerBounceTest.testSeekAndCommitWithBrokerFailures ``` I don't notice related error, and they pass on my local. Will merge 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
Re: [PR] Kafka-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1601756235
##
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:
thanks for the comment! let me change that
--
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-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1601754643
##
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:
`Arrays.asList` is used to create a list with many items.
--
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-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1601753901
##
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:
thanks for the comment! let me add that
--
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-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1601750727
##
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:
thanks for the comment!
`singletonList` is a more flexible wrapper of a list, that's the reason why
it's better than just using Arrays, 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-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1601745613
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -153,13 +153,13 @@ public Map>
perServerOverrideProperties() {
return perServerProperties;
}
-public Optional> tags() {
-return Optional.ofNullable(tags);
+public List tags() {
+return tags;
}
public Set displayTags() {
Set displayTags = new LinkedHashSet<>(tags);
-tags().ifPresent(tags -> displayTags.add("tags=" + tags));
+displayTags.add("tags=" + String.join(",", tags));
Review Comment:
oops yes you are right, let me remove that
--
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-16668: Add tags support in ClusterTestExtension [kafka]
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>
perServerOverrideProperties() {
return perServerProperties;
}
-public Optional> tags() {
-return Optional.ofNullable(tags);
+public List tags() {
+return tags;
}
public Set displayTags() {
Set 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:
Re: [PR] Kafka-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on PR #15861: URL: https://github.com/apache/kafka/pull/15861#issuecomment-2106748277 @johnnychhsu Please fix the conflicts. -- 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-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on PR #15861: URL: https://github.com/apache/kafka/pull/15861#issuecomment-2105935810 @chia7712 thanks for the quick review! -- 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-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1597465635
##
core/src/test/java/kafka/test/junit/RaftClusterInvocationContext.java:
##
@@ -79,7 +79,7 @@ public RaftClusterInvocationContext(String baseDisplayName,
ClusterConfig cluste
@Override
public String getDisplayName(int invocationIndex) {
-String clusterDesc = clusterConfig.nameTags().entrySet().stream()
+String clusterDesc = clusterConfig.displayTags().stream()
.map(Object::toString)
.collect(Collectors.joining(", "));
return String.format("%s [%d] Type=Raft-%s, %s", baseDisplayName,
invocationIndex, isCombined ? "Combined" : "Isolated", clusterDesc);
Review Comment:
`return String.format("%s [%d] Type=Raft-%s, %s", baseDisplayName,
invocationIndex, isCombined ? "Combined" : "Isolated", String.join(",",
clusterConfig.displayTags()));`
##
core/src/test/java/kafka/test/junit/ZkClusterInvocationContext.java:
##
@@ -81,7 +81,7 @@ public ZkClusterInvocationContext(String baseDisplayName,
ClusterConfig clusterC
@Override
public String getDisplayName(int invocationIndex) {
-String clusterDesc = clusterConfig.nameTags().entrySet().stream()
+String clusterDesc = clusterConfig.displayTags().stream()
.map(Object::toString)
.collect(Collectors.joining(", "));
return String.format("%s [%d] Type=ZK, %s", baseDisplayName,
invocationIndex, clusterDesc);
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
Re: [PR] Kafka-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1597465561
##
tools/src/test/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommandTestUtils.java:
##
@@ -55,19 +56,22 @@ static void generator(ClusterGenerator clusterGenerator) {
ClusterConfig zk = ClusterConfig.defaultBuilder()
.setType(ZK)
+.setTags(Collections.singletonList("newGroupCoordinator"))
Review Comment:
this should be `classicGroupCoordinator`
--
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-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1597465579
##
tools/src/test/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommandTestUtils.java:
##
@@ -55,19 +56,22 @@ static void generator(ClusterGenerator clusterGenerator) {
ClusterConfig zk = ClusterConfig.defaultBuilder()
.setType(ZK)
+.setTags(Collections.singletonList("newGroupCoordinator"))
.setServerProperties(serverProperties)
.build();
clusterGenerator.accept(zk);
ClusterConfig raftWithLegacyCoordinator =
ClusterConfig.defaultBuilder()
.setType(KRAFT)
+.setTags(Collections.singletonList("newGroupCoordinator"))
.setServerProperties(serverProperties)
.build();
clusterGenerator.accept(raftWithLegacyCoordinator);
ClusterConfig combinedKRaftWithLegacyCoordinator =
ClusterConfig.defaultBuilder()
.setType(CO_KRAFT)
.setServerProperties(serverProperties)
+.setTags(Collections.singletonList("newGroupCoordinator"))
Review Comment:
ditto
##
tools/src/test/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommandTestUtils.java:
##
@@ -55,19 +56,22 @@ static void generator(ClusterGenerator clusterGenerator) {
ClusterConfig zk = ClusterConfig.defaultBuilder()
.setType(ZK)
+.setTags(Collections.singletonList("newGroupCoordinator"))
.setServerProperties(serverProperties)
.build();
clusterGenerator.accept(zk);
ClusterConfig raftWithLegacyCoordinator =
ClusterConfig.defaultBuilder()
.setType(KRAFT)
+.setTags(Collections.singletonList("newGroupCoordinator"))
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
Re: [PR] Kafka-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on code in PR #15861: URL: https://github.com/apache/kafka/pull/15861#discussion_r1597465415 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -151,19 +151,19 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu .filter(e -> e.id() != -1) .collect(Collectors.groupingBy(ClusterConfigProperty::id, Collectors.mapping(Function.identity(), Collectors.toMap(ClusterConfigProperty::key, ClusterConfigProperty::value, (a, b) -> b; - +List tags = new ArrayList<>(Arrays.asList(annot.tags())); ClusterConfig config = ClusterConfig.builder() .setType(type) .setBrokers(annot.brokers() == 0 ? defaults.brokers() : annot.brokers()) .setControllers(annot.controllers() == 0 ? defaults.controllers() : annot.controllers()) .setDisksPerBroker(annot.disksPerBroker() == 0 ? defaults.disksPerBroker() : annot.disksPerBroker()) .setAutoStart(annot.autoStart() == AutoStart.DEFAULT ? defaults.autoStart() : annot.autoStart() == AutoStart.YES) -.setName(annot.name().trim().isEmpty() ? null : annot.name()) .setListenerName(annot.listener().trim().isEmpty() ? null : annot.listener()) .setServerProperties(serverProperties) .setPerServerProperties(perServerProperties) .setSecurityProtocol(annot.securityProtocol()) .setMetadataVersion(annot.metadataVersion()) +.setTags(tags) Review Comment: `setTags(Arrays.asList(annot.tags()))` -- 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-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1597465331
##
core/src/test/java/kafka/test/ClusterConfigTest.java:
##
@@ -44,14 +45,16 @@ private static Map fields(ClusterConfig
config) {
@Test
public void testCopy() throws IOException {
File trustStoreFile = TestUtils.tempFile();
+String[] array = {"name", "Generated Test"};
Review Comment:
those are unused.
--
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-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1597465290
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -153,13 +150,17 @@ public Map>
perBrokerOverrideProperties() {
return perBrokerOverrideProperties;
}
-public Map nameTags() {
-Map tags = new LinkedHashMap<>(4);
-name().ifPresent(name -> tags.put("Name", name));
-tags.put("MetadataVersion", metadataVersion.toString());
-tags.put("Security", securityProtocol.name());
-listenerName().ifPresent(listener -> tags.put("Listener", listener));
-return tags;
+public Optional> tags() {
Review Comment:
@johnnychhsu it seems you don't fix this actually.
--
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-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on PR #15861: URL: https://github.com/apache/kafka/pull/15861#issuecomment-2105669617 test with `./gradlew clean tools:test --tests DeleteConsumerGroupsTest --tests DeleteOffsetsConsumerGroupCommandIntegrationTest` and `./gradlew clean core:test --tests ClusterTestExtensionsTest --tests ClusterConfigTest --tests ClusterTestExtensionsTest` and passed. -- 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-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on code in PR #15861: URL: https://github.com/apache/kafka/pull/15861#discussion_r1597412007 ## core/src/test/java/kafka/test/ClusterConfig.java: ## @@ -83,6 +83,7 @@ private ClusterConfig(Type type, int brokers, int controllers, int disksPerBroke this.saslServerProperties = Objects.requireNonNull(saslServerProperties); this.saslClientProperties = Objects.requireNonNull(saslClientProperties); this.perBrokerOverrideProperties = Objects.requireNonNull(perBrokerOverrideProperties); +this.tags = tags; Review Comment: originally I use `Objects.requireNonNull(tags);` but I was not sure if we should make this required since users might not need this, so basically we want/expect users to provide for the display purpose, 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-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1594882664
##
tools/src/test/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommandTestUtils.java:
##
@@ -74,16 +75,18 @@ static void generator(ClusterGenerator clusterGenerator) {
// Following are test case config with new group coordinator
serverProperties.put(NEW_GROUP_COORDINATOR_ENABLE_CONFIG, "true");
+String[] array = {"newGroupCoordinator"};
+ArrayList tags = new ArrayList<>(Arrays.asList(array));
ClusterConfig raftWithNewGroupCoordinator =
ClusterConfig.defaultBuilder()
.setType(KRAFT)
-.setName("newGroupCoordinator")
+.setTags(tags)
Review Comment:
` .setTags(Collections.singletonList("newGroupCoordinator"))`
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -306,11 +302,16 @@ public Builder setPerBrokerProperties(Map> perBroke
return this;
}
+public Builder setTags(List tags) {
+this.tags = tags;
Review Comment:
`this.tags = Collections.unmodifiableList(new ArrayList<>(tags));`
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -153,13 +150,17 @@ public Map>
perBrokerOverrideProperties() {
return perBrokerOverrideProperties;
}
-public Map nameTags() {
-Map tags = new LinkedHashMap<>(4);
-name().ifPresent(name -> tags.put("Name", name));
-tags.put("MetadataVersion", metadataVersion.toString());
-tags.put("Security", securityProtocol.name());
-listenerName().ifPresent(listener -> tags.put("Listener", listener));
-return tags;
+public Optional> tags() {
Review Comment:
```java
public List tags() {
return tags;
}
```
##
core/src/test/java/kafka/test/ClusterConfigTest.java:
##
@@ -44,13 +45,15 @@ private static Map fields(ClusterConfig
config) {
@Test
public void testCopy() throws IOException {
File trustStoreFile = TestUtils.tempFile();
+String[] array = {"name", "Generated Test"};
+ArrayList tags = new ArrayList<>(Arrays.asList(array));
ClusterConfig clusterConfig = ClusterConfig.builder()
.setType(Type.KRAFT)
.setBrokers(3)
.setControllers(2)
.setDisksPerBroker(1)
-.setName("builder-test")
+.setTags(tags)
Review Comment:
`Arrays.asList("name", "Generated Test")`
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -83,6 +83,7 @@ private ClusterConfig(Type type, int brokers, int
controllers, int disksPerBroke
this.saslServerProperties =
Objects.requireNonNull(saslServerProperties);
this.saslClientProperties =
Objects.requireNonNull(saslClientProperties);
this.perBrokerOverrideProperties =
Objects.requireNonNull(perBrokerOverrideProperties);
+this.tags = tags;
Review Comment:
`Objects.requireNonNull(tags);`
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -153,13 +150,17 @@ public Map>
perBrokerOverrideProperties() {
return perBrokerOverrideProperties;
}
-public Map nameTags() {
-Map tags = new LinkedHashMap<>(4);
-name().ifPresent(name -> tags.put("Name", name));
-tags.put("MetadataVersion", metadataVersion.toString());
-tags.put("Security", securityProtocol.name());
-listenerName().ifPresent(listener -> tags.put("Listener", listener));
-return tags;
+public Optional> tags() {
+return Optional.ofNullable(tags);
+}
+
+public Set displayTags() {
+Set displayTags = new LinkedHashSet<>(4);
Review Comment:
`Set displayTags = new LinkedHashSet<>(tags);`
--
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-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on PR #15861: URL: https://github.com/apache/kafka/pull/15861#issuecomment-2100511022 run `./gradlew clean core:test --tests ClusterTestExtensionsTest --tests ClusterConfigTest` and passed -- 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-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1593920627
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -153,15 +148,19 @@ public Map>
perBrokerOverrideProperties() {
return perBrokerOverrideProperties;
}
-public Map nameTags() {
-Map tags = new LinkedHashMap<>(4);
-name().ifPresent(name -> tags.put("Name", name));
-tags.put("MetadataVersion", metadataVersion.toString());
-tags.put("Security", securityProtocol.name());
-listenerName().ifPresent(listener -> tags.put("Listener", listener));
+public String[] tags() {
return tags;
}
+public Map nameTags() {
Review Comment:
I see, basically also change the current nameTags to a List instead of a
map.
sure, let me address this, 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-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1593917049
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -53,14 +52,15 @@ public class ClusterConfig {
private final Map adminClientProperties;
private final Map saslServerProperties;
private final Map saslClientProperties;
+private final ArrayList tags;
Review Comment:
ah I see, thanks for the review!
--
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-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1593356187
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -53,14 +52,15 @@ public class ClusterConfig {
private final Map adminClientProperties;
private final Map saslServerProperties;
private final Map saslClientProperties;
+private final ArrayList tags;
Review Comment:
Please use interface `List> instead
--
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-16668: Add tags support in ClusterTestExtension [kafka]
chia7712 commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1593355835
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -153,15 +148,19 @@ public Map>
perBrokerOverrideProperties() {
return perBrokerOverrideProperties;
}
-public Map nameTags() {
-Map tags = new LinkedHashMap<>(4);
-name().ifPresent(name -> tags.put("Name", name));
-tags.put("MetadataVersion", metadataVersion.toString());
-tags.put("Security", securityProtocol.name());
-listenerName().ifPresent(listener -> tags.put("Listener", listener));
+public String[] tags() {
return tags;
}
+public Map nameTags() {
Review Comment:
My point was `Map nameTags` should be changed to
`Set nameTags` (or `displayTags`). Also, it should include the "tags".
For example:
```java
public Set nameTags() {
Set nameTags = new LinkedHashSet<>(tags);
nameTags.add("MetadataVersion=" + metadataVersion.toString());
nameTags.add("Security=" + securityProtocol.name());
listenerName().ifPresent(listener -> nameTags.add("Listener=" +
listener));
return nameTags;
}
```
--
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-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on PR #15861: URL: https://github.com/apache/kafka/pull/15861#issuecomment-2098850247 @chia7712 > As https://github.com/apache/kafka/pull/15766 gets merged now, could you please fix the build error? sure, let me rebase the latest trunk and fix that since I remove the name there -- 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-16668: Add tags support in ClusterTestExtension [kafka]
johnnychhsu commented on code in PR #15861:
URL: https://github.com/apache/kafka/pull/15861#discussion_r1592759299
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -153,15 +148,19 @@ public Map>
perBrokerOverrideProperties() {
return perBrokerOverrideProperties;
}
-public Map nameTags() {
-Map tags = new LinkedHashMap<>(4);
-name().ifPresent(name -> tags.put("Name", name));
-tags.put("MetadataVersion", metadataVersion.toString());
-tags.put("Security", securityProtocol.name());
-listenerName().ifPresent(listener -> tags.put("Listener", listener));
+public String[] tags() {
return tags;
}
+public Map nameTags() {
Review Comment:
since we can get the tags from the getter, why do we still need to add this
into the displayTags?
--
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
