Re: [PR] KAFKA-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Rancho-7 commented on PR #17771: URL: https://github.com/apache/kafka/pull/17771#issuecomment-2490245148 @chia7712 PTAL. -- 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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Rancho-7 commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1849500672
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,36 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
Review Comment:
Okay.
> Nit: `String topic = "test-topic";` and replace all "test-topic" with
`topic`.
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
TaiJuWu commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1850347152
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,36 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
Review Comment:
My meaning is using variable to replace constant but this PR is good enough.
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Rancho-7 commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1850237251
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,36 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+cluster.waitForTopic("test-topic", 1);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--alter", "--add-config",
"cleanup.policy=[delete,compact]"));
+
+String message = captureStandardStream(false, run(command));
+assertEquals("Completed updating config for topic test-topic.",
message);
+
+command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--describe"));
+
+message = captureStandardStream(false, run(command));
+assertTrue(message.contains("cleanup.policy=delete,compact"),
"Config entry was not added correctly");
+
+//Clean up by deleting topic
+
client.deleteTopics(Collections.singleton("test-topic")).all().get();
Review Comment:
> We also need to make sure topic already deleted here.
`cluster.waitForTopic("test-topic, 0);`
Hi,I find that the topic will be deleted automatically,so may be there is no
need to do that manually?
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Yunyung commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1850350465
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,34 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+cluster.waitForTopic("topic", 1);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "topic",
+"--alter", "--add-config",
"cleanup.policy=[delete,compact]"));
+
+String message = captureStandardStream(false, run(command));
+assertEquals("Completed updating config for topic topic.",
message);
+
+command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "topic",
+"--describe"));
+
+message = captureStandardStream(false, run(command));
+assertTrue(message.contains("cleanup.policy=delete,compact"),
"Config entry was not added correctly");
+
+}
+
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Yunyung commented on PR #17771: URL: https://github.com/apache/kafka/pull/17771#issuecomment-2488660050 LGTM. -- 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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Rancho-7 commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1850355256
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,34 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+cluster.waitForTopic("topic", 1);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "topic",
+"--alter", "--add-config",
"cleanup.policy=[delete,compact]"));
+
+String message = captureStandardStream(false, run(command));
+assertEquals("Completed updating config for topic topic.",
message);
+
+command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "topic",
+"--describe"));
+
+message = captureStandardStream(false, run(command));
+assertTrue(message.contains("cleanup.policy=delete,compact"),
"Config entry was not added correctly");
+
Review Comment:
> nit: delete extra line.
ok
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Yunyung commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1850348815
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,34 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+cluster.waitForTopic("topic", 1);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "topic",
+"--alter", "--add-config",
"cleanup.policy=[delete,compact]"));
+
+String message = captureStandardStream(false, run(command));
+assertEquals("Completed updating config for topic topic.",
message);
+
+command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "topic",
+"--describe"));
+
+message = captureStandardStream(false, run(command));
+assertTrue(message.contains("cleanup.policy=delete,compact"),
"Config entry was not added correctly");
+
Review Comment:
nit: delete extra line.
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
TaiJuWu commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1850344139
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,36 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+cluster.waitForTopic("test-topic", 1);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--alter", "--add-config",
"cleanup.policy=[delete,compact]"));
+
+String message = captureStandardStream(false, run(command));
+assertEquals("Completed updating config for topic test-topic.",
message);
+
+command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--describe"));
+
+message = captureStandardStream(false, run(command));
+assertTrue(message.contains("cleanup.policy=delete,compact"),
"Config entry was not added correctly");
+
+//Clean up by deleting topic
+
client.deleteTopics(Collections.singleton("test-topic")).all().get();
Review Comment:
You are right. `clusterInstance` will delete dir automatically.
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Rancho-7 commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1849499551
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,36 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+cluster.waitForTopic("test-topic", 1);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--alter", "--add-config",
"cleanup.policy=[delete,compact]"));
+
+String message = captureStandardStream(false, run(command));
+assertEquals("Completed updating config for topic test-topic.",
message);
+
+command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--describe"));
+
+message = captureStandardStream(false, run(command));
+assertTrue(message.contains("cleanup.policy=delete,compact"),
"Config entry was not added correctly");
+
+//Clean up by deleting topic
+
client.deleteTopics(Collections.singleton("test-topic")).all().get();
Review Comment:
Yes,you are right. 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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
TaiJuWu commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1849404649
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,36 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
Review Comment:
Nit: `String topic = "test-topic";` and replace all "test-topic" with
`topic`.
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,36 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+cluster.waitForTopic("test-topic", 1);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--alter", "--add-config",
"cleanup.policy=[delete,compact]"));
+
+String message = captureStandardStream(false, run(command));
+assertEquals("Completed updating config for topic test-topic.",
message);
+
+command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--describe"));
+
+message = captureStandardStream(false, run(command));
+assertTrue(message.contains("cleanup.policy=delete,compact"),
"Config entry was not added correctly");
+
+//Clean up by deleting topic
+
client.deleteTopics(Collections.singleton("test-topic")).all().get();
Review Comment:
We also need to make sure topic already deleted here.
`cluster.waitForTopic("test-topic, 0);`
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Rancho-7 commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1849375257
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,37 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+cluster.waitForTopic("test-topic", 1);
+}
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--alter", "--add-config", "cleanup.policy=[delete,compact]"));
+
+String message = captureStandardStream(false, run(command));
+assertEquals("Completed updating config for topic test-topic.",
message);
+
+command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--describe"));
+
+message = captureStandardStream(false, run(command));
+assertTrue(message.contains("cleanup.policy=delete,compact"), "Config
entry was not added correctly");
+
+try (Admin client = cluster.admin()) {
+
client.deleteTopics(Collections.singleton("test-topic")).all().get();
Review Comment:
Seems good.I will adjust my code.Thanks!
> ditto. Additionally, we can reuse the same `admin` to avoid creating admin
two times.
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
TaiJuWu commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1848500117
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,36 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+}
+
Review Comment:
We need to make sure topic is created here.
You can use
https://github.com/apache/kafka/blob/53d8316b5da7d8355331aa2d50193748c8261706/test-common/test-common-api/src/main/java/org/apache/kafka/common/test/api/ClusterInstance.java#L250
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
TaiJuWu commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1848693145
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,37 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+cluster.waitForTopic("test-topic", 1);
+}
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--alter", "--add-config", "cleanup.policy=[delete,compact]"));
+
+String message = captureStandardStream(false, run(command));
+assertEquals("Completed updating config for topic test-topic.",
message);
+
+command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--describe"));
+
+message = captureStandardStream(false, run(command));
+assertTrue(message.contains("cleanup.policy=delete,compact"), "Config
entry was not added correctly");
+
+try (Admin client = cluster.admin()) {
+
client.deleteTopics(Collections.singleton("test-topic")).all().get();
Review Comment:
ditto.
Additionally, we can reuse the same `admin` to avoid creating admin two
times.
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
TaiJuWu commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1848693145
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,37 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+cluster.waitForTopic("test-topic", 1);
+}
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--alter", "--add-config", "cleanup.policy=[delete,compact]"));
+
+String message = captureStandardStream(false, run(command));
+assertEquals("Completed updating config for topic test-topic.",
message);
+
+command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", "test-topic",
+"--describe"));
+
+message = captureStandardStream(false, run(command));
+assertTrue(message.contains("cleanup.policy=delete,compact"), "Config
entry was not added correctly");
+
+try (Admin client = cluster.admin()) {
+
client.deleteTopics(Collections.singleton("test-topic")).all().get();
Review Comment:
ditto.
Additionally, we can reuse the same `admin` to avoid create admin two times.
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
TaiJuWu commented on PR #17771: URL: https://github.com/apache/kafka/pull/17771#issuecomment-2486214661 Thanks for the patch. I leave some comments. -- 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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Rancho-7 commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1848522628
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,36 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+
+try (Admin client = cluster.admin()) {
+NewTopic newTopic = new NewTopic("test-topic", 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+}
+
Review Comment:
OK.Thanks.I will fix 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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Rancho-7 commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1838048135
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,41 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+String topicName = "test-topic";
+
+try (Admin client = cluster.createAdminClient()) {
+NewTopic newTopic = new NewTopic(topicName, 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+}
+
+String configKey = "cleanup.policy";
+List configValues = List.of("delete", "compact");
+String configValueStr = String.join(",", configValues);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", topicName,
+"--alter", "--add-config", configKey + "=" + "[" +
configValueStr + "]"));
Review Comment:
done.
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,41 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+String topicName = "test-topic";
+
+try (Admin client = cluster.createAdminClient()) {
+NewTopic newTopic = new NewTopic(topicName, 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+}
+
+String configKey = "cleanup.policy";
+List configValues = List.of("delete", "compact");
+String configValueStr = String.join(",", configValues);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", topicName,
+"--alter", "--add-config", configKey + "=" + "[" +
configValueStr + "]"));
+
+String output = captureStandardStream(false, run(command));
Review Comment:
done.
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Rancho-7 commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1838047790
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,41 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+String topicName = "test-topic";
+
+try (Admin client = cluster.createAdminClient()) {
+NewTopic newTopic = new NewTopic(topicName, 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+}
+
+String configKey = "cleanup.policy";
+List configValues = List.of("delete", "compact");
+String configValueStr = String.join(",", configValues);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", topicName,
Review Comment:
ok.done.
--
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Yunyung commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1837995366
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,41 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+String topicName = "test-topic";
+
+try (Admin client = cluster.createAdminClient()) {
+NewTopic newTopic = new NewTopic(topicName, 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+}
+
+String configKey = "cleanup.policy";
+List configValues = List.of("delete", "compact");
+String configValueStr = String.join(",", configValues);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", topicName,
+"--alter", "--add-config", configKey + "=" + "[" +
configValueStr + "]"));
+
+String output = captureStandardStream(false, run(command));
Review Comment:
Change naming output -> message.
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,41 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+String topicName = "test-topic";
+
+try (Admin client = cluster.createAdminClient()) {
+NewTopic newTopic = new NewTopic(topicName, 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+}
+
+String configKey = "cleanup.policy";
+List configValues = List.of("delete", "compact");
+String configValueStr = String.join(",", configValues);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", topicName,
+"--alter", "--add-config", configKey + "=" + "[" +
configValueStr + "]"));
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-17981:add Integration test for ConfigCommand to add config `key=[val1,val2]` [kafka]
Yunyung commented on code in PR #17771:
URL: https://github.com/apache/kafka/pull/17771#discussion_r1837993667
##
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##
@@ -138,6 +139,41 @@ public void
testNullStatusOnKraftCommandAlterClientMetrics() {
assertEquals("Completed updating config for client-metric cm.",
message);
}
+@ClusterTest
+public void testAddConfigKeyValuesUsingCommand() throws Exception {
+String topicName = "test-topic";
+
+try (Admin client = cluster.createAdminClient()) {
+NewTopic newTopic = new NewTopic(topicName, 1, (short) 1);
+client.createTopics(Collections.singleton(newTopic)).all().get();
+}
+
+String configKey = "cleanup.policy";
+List configValues = List.of("delete", "compact");
+String configValueStr = String.join(",", configValues);
+
+Stream command = Stream.concat(quorumArgs(), Stream.of(
+"--entity-type", "topics",
+"--entity-name", topicName,
Review Comment:
To follow the coding style in this file, like in
`testNullStatusOnKraftCommandAlterClientMetrics`, it’s better to explicitly
list constant strings instead of storing them in variables.
--
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
