Re: [PR] KAFKA-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
chia7712 merged PR #16355: URL: https://github.com/apache/kafka/pull/16355 -- 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-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
frankvicky commented on PR #16355: URL: https://github.com/apache/kafka/pull/16355#issuecomment-2172550434 Hi @chia7712, I have refactor the test case, 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-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
chia7712 commented on code in PR #16355:
URL: https://github.com/apache/kafka/pull/16355#discussion_r1642017586
##
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##
@@ -1001,6 +1001,29 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
assertTrue(assertThrows(classOf[ExecutionException], () =>
describeResult2.values.get(invalidTopic).get).getCause.isInstanceOf[InvalidTopicException])
}
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk", "kraft"))
+ def testIncludeDocumentation(quorum: String): Unit = {
+createTopic(topic)
+client = createAdminClient
+
+val resources = Collections.singletonList(new
ConfigResource(ConfigResource.Type.TOPIC, topic))
+val includeDescribe = new
DescribeConfigsOptions().includeDocumentation(true)
+var describeConfigs = client.describeConfigs(resources, includeDescribe)
+val pattern = """documentation=([^,]*?)\)""".r
+var resourceToConfig = describeConfigs.all().get()
+var matches = pattern.findAllMatchIn(resourceToConfig.toString)
Review Comment:
Could you please verify `ConfigEntry#documentation` too?
--
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-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
frankvicky commented on PR #16355: URL: https://github.com/apache/kafka/pull/16355#issuecomment-2171469766 Hi @chia7712, I have make a change based on your comment, 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-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
chia7712 commented on code in PR #16355:
URL: https://github.com/apache/kafka/pull/16355#discussion_r1641728889
##
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##
@@ -1001,6 +1001,30 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
assertTrue(assertThrows(classOf[ExecutionException], () =>
describeResult2.values.get(invalidTopic).get).getCause.isInstanceOf[InvalidTopicException])
}
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk", "kraft"))
+ def testIncludeDocumentation(quorum: String): Unit = {
+val consumer = createConsumer()
+subscribeAndWaitForAssignment(topic, consumer)
Review Comment:
Could you please use `admin` to create topic instead of creating consumer?
--
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-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
frankvicky commented on PR #16355: URL: https://github.com/apache/kafka/pull/16355#issuecomment-2170997483 Hi @TaiJuWu I have make a change about assertions, 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-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
frankvicky commented on code in PR #16355:
URL: https://github.com/apache/kafka/pull/16355#discussion_r1641587447
##
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##
@@ -1001,6 +1001,30 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
assertTrue(assertThrows(classOf[ExecutionException], () =>
describeResult2.values.get(invalidTopic).get).getCause.isInstanceOf[InvalidTopicException])
}
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk", "kraft"))
+ def testIncludeDocumentation(quorum: String): Unit = {
+val consumer = createConsumer()
+subscribeAndWaitForAssignment(topic, consumer)
+client = createAdminClient
+
+val resource = new ConfigResource(ConfigResource.Type.TOPIC, topic)
+val includeDescribe = new
DescribeConfigsOptions().includeDocumentation(true)
+var describeConfigs =
client.describeConfigs(Collections.singletonList(resource), includeDescribe)
+val pattern = """documentation=([^,]*?)\)""".r
+var resourceToConfig = describeConfigs.all().get()
+var matches = pattern.findAllMatchIn(resourceToConfig.toString)
+var describes = matches.map(e => e.group(1)).toList
+describes.foreach(e => assertNotNull(e))
Review Comment:
Yes, you're right, in this case it will the `null` of string literal, not
actually `null`
--
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-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
frankvicky commented on code in PR #16355:
URL: https://github.com/apache/kafka/pull/16355#discussion_r1641587398
##
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##
@@ -1001,6 +1001,30 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
assertTrue(assertThrows(classOf[ExecutionException], () =>
describeResult2.values.get(invalidTopic).get).getCause.isInstanceOf[InvalidTopicException])
}
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk", "kraft"))
+ def testIncludeDocumentation(quorum: String): Unit = {
+val consumer = createConsumer()
+subscribeAndWaitForAssignment(topic, consumer)
+client = createAdminClient
+
+val resource = new ConfigResource(ConfigResource.Type.TOPIC, topic)
+val includeDescribe = new
DescribeConfigsOptions().includeDocumentation(true)
+var describeConfigs =
client.describeConfigs(Collections.singletonList(resource), includeDescribe)
+val pattern = """documentation=([^,]*?)\)""".r
Review Comment:
IMHO, it's more precise and robust as it avoids matching commas, ensuring
the `documentation` value is accurately captured up to the first comma or
closing.
But it will be more complicated than `[^,]`
--
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-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
TaiJuWu commented on code in PR #16355:
URL: https://github.com/apache/kafka/pull/16355#discussion_r1641243077
##
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##
@@ -1001,6 +1001,30 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
assertTrue(assertThrows(classOf[ExecutionException], () =>
describeResult2.values.get(invalidTopic).get).getCause.isInstanceOf[InvalidTopicException])
}
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk", "kraft"))
+ def testIncludeDocumentation(quorum: String): Unit = {
+val consumer = createConsumer()
+subscribeAndWaitForAssignment(topic, consumer)
+client = createAdminClient
+
+val resource = new ConfigResource(ConfigResource.Type.TOPIC, topic)
+val includeDescribe = new
DescribeConfigsOptions().includeDocumentation(true)
+var describeConfigs =
client.describeConfigs(Collections.singletonList(resource), includeDescribe)
+val pattern = """documentation=([^,]*?)\)""".r
Review Comment:
Could you explain why we need `[^,]` instead of `documentation=(.*?)\)`?
--
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-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
TaiJuWu commented on code in PR #16355:
URL: https://github.com/apache/kafka/pull/16355#discussion_r1641243077
##
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##
@@ -1001,6 +1001,30 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
assertTrue(assertThrows(classOf[ExecutionException], () =>
describeResult2.values.get(invalidTopic).get).getCause.isInstanceOf[InvalidTopicException])
}
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk", "kraft"))
+ def testIncludeDocumentation(quorum: String): Unit = {
+val consumer = createConsumer()
+subscribeAndWaitForAssignment(topic, consumer)
+client = createAdminClient
+
+val resource = new ConfigResource(ConfigResource.Type.TOPIC, topic)
+val includeDescribe = new
DescribeConfigsOptions().includeDocumentation(true)
+var describeConfigs =
client.describeConfigs(Collections.singletonList(resource), includeDescribe)
+val pattern = """documentation=([^,]*?)\)""".r
Review Comment:
Could you explain why we need `[^,]`?
--
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-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
TaiJuWu commented on code in PR #16355:
URL: https://github.com/apache/kafka/pull/16355#discussion_r1641243077
##
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##
@@ -1001,6 +1001,30 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
assertTrue(assertThrows(classOf[ExecutionException], () =>
describeResult2.values.get(invalidTopic).get).getCause.isInstanceOf[InvalidTopicException])
}
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk", "kraft"))
+ def testIncludeDocumentation(quorum: String): Unit = {
+val consumer = createConsumer()
+subscribeAndWaitForAssignment(topic, consumer)
+client = createAdminClient
+
+val resource = new ConfigResource(ConfigResource.Type.TOPIC, topic)
+val includeDescribe = new
DescribeConfigsOptions().includeDocumentation(true)
+var describeConfigs =
client.describeConfigs(Collections.singletonList(resource), includeDescribe)
+val pattern = """documentation=([^,]*?)\)""".r
Review Comment:
How about `documentation=(.*\)?)`
--
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-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
TaiJuWu commented on code in PR #16355:
URL: https://github.com/apache/kafka/pull/16355#discussion_r1641231798
##
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##
@@ -1001,6 +1001,30 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
assertTrue(assertThrows(classOf[ExecutionException], () =>
describeResult2.values.get(invalidTopic).get).getCause.isInstanceOf[InvalidTopicException])
}
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk", "kraft"))
+ def testIncludeDocumentation(quorum: String): Unit = {
+val consumer = createConsumer()
+subscribeAndWaitForAssignment(topic, consumer)
+client = createAdminClient
+
+val resource = new ConfigResource(ConfigResource.Type.TOPIC, topic)
+val includeDescribe = new
DescribeConfigsOptions().includeDocumentation(true)
+var describeConfigs =
client.describeConfigs(Collections.singletonList(resource), includeDescribe)
+val pattern = """documentation=([^,]*?)\)""".r
+var resourceToConfig = describeConfigs.all().get()
+var matches = pattern.findAllMatchIn(resourceToConfig.toString)
+var describes = matches.map(e => e.group(1)).toList
+describes.foreach(e => assertNotNull(e))
Review Comment:
Maybe this should be `assertNotEquals("null", e)`
assertNotNull is used to check the object is null.
--
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
[PR] KAFKA-16547: Add test for DescribeConfigsOptions#includeDocumentation [kafka]
frankvicky opened a new pull request, #16355: URL: https://github.com/apache/kafka/pull/16355 We currently do not have tests for the `includeDocumentation` query option. If the option is set to false, `ConfigEntry[documentation]` should be null. Otherwise, it should return the configuration documentation. ### Committer Checklist (excluded from commit message) - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes) -- 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
