You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/10/01 11:26:40 UTC

[GitHub] [kafka] edoardocomar opened a new pull request #9360: KAFKA-10557: Missing docs when describing topic configs including

edoardocomar opened a new pull request #9360:
URL: https://github.com/apache/kafka/pull/9360


   documentation
   
   Added unit test
   
   Co-authored-by: Edoardo Comar <ec...@uk.ibm.com>
   Co-authored-by: Mickael Maison <mi...@gmail.com>
   
   ### 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [kafka] edoardocomar commented on pull request #9360: KAFKA-10557: Missing docs when describing topic configs including

Posted by GitBox <gi...@apache.org>.
edoardocomar commented on pull request #9360:
URL: https://github.com/apache/kafka/pull/9360#issuecomment-702070253


   @rajinisivaram may you take a look please ?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [kafka] mimaison commented on pull request #9360: KAFKA-10557: Missing docs when describing topic configs including

Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #9360:
URL: https://github.com/apache/kafka/pull/9360#issuecomment-702614729


   Thanks @rajinisivaram for the review.
   Test failures are not related, merging.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [kafka] edoardocomar commented on a change in pull request #9360: KAFKA-10557: Missing docs when describing topic configs including

Posted by GitBox <gi...@apache.org>.
edoardocomar commented on a change in pull request #9360:
URL: https://github.com/apache/kafka/pull/9360#discussion_r498363295



##########
File path: core/src/test/scala/unit/kafka/server/AdminManagerTest.scala
##########
@@ -80,4 +81,30 @@ class AdminManagerTest {
     assertEquals(Errors.NONE.code, results.head.errorCode())
     assertFalse("Should return configs", results.head.configs().isEmpty)
   }
+
+  @Test
+  def testDescribeConfigsWithDocumentation(): Unit = {
+    EasyMock.expect(zkClient.getEntityConfigs(ConfigType.Topic, topic)).andReturn(new java.util.Properties)
+    EasyMock.expect(zkClient.getEntityConfigs(ConfigType.Broker, brokerId.toString)).andReturn(TestUtils.createBrokerConfig(brokerId,"zk"))
+    EasyMock.expect(metadataCache.contains(topic)).andReturn(true)
+    EasyMock.replay(zkClient, metadataCache)
+
+    val adminManager = createAdminManager()
+
+    val resources = List(
+      new DescribeConfigsRequestData.DescribeConfigsResource()
+        .setResourceName(topic)
+        .setResourceType(ConfigResource.Type.TOPIC.id),
+      new DescribeConfigsRequestData.DescribeConfigsResource()
+        .setResourceName(brokerId.toString)
+        .setResourceType(ConfigResource.Type.BROKER.id))
+
+    val results: List[DescribeConfigsResponseData.DescribeConfigsResult] = adminManager.describeConfigs(resources, true, true)
+    assertEquals(2, results.size)
+    results.foreach(r => {
+      assertEquals(Errors.NONE.code, r.errorCode)
+      assertFalse("Should return configs", r.configs.isEmpty)
+      r.configs.forEach(c => assertNotNull(s"Config ${c.name} should have documentation", c.documentation))

Review comment:
       sure 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [kafka] mimaison commented on pull request #9360: KAFKA-10557: Missing docs when describing topic configs including

Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #9360:
URL: https://github.com/apache/kafka/pull/9360#issuecomment-702617314


   I'll also merge into 2.6


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [kafka] rajinisivaram commented on a change in pull request #9360: KAFKA-10557: Missing docs when describing topic configs including

Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on a change in pull request #9360:
URL: https://github.com/apache/kafka/pull/9360#discussion_r498322557



##########
File path: core/src/test/scala/unit/kafka/server/AdminManagerTest.scala
##########
@@ -80,4 +81,30 @@ class AdminManagerTest {
     assertEquals(Errors.NONE.code, results.head.errorCode())
     assertFalse("Should return configs", results.head.configs().isEmpty)
   }
+
+  @Test
+  def testDescribeConfigsWithDocumentation(): Unit = {
+    EasyMock.expect(zkClient.getEntityConfigs(ConfigType.Topic, topic)).andReturn(new java.util.Properties)
+    EasyMock.expect(zkClient.getEntityConfigs(ConfigType.Broker, brokerId.toString)).andReturn(TestUtils.createBrokerConfig(brokerId,"zk"))

Review comment:
       nit: space before "zk" - why is broker id anyway :-) ?

##########
File path: core/src/test/scala/unit/kafka/server/AdminManagerTest.scala
##########
@@ -80,4 +81,30 @@ class AdminManagerTest {
     assertEquals(Errors.NONE.code, results.head.errorCode())
     assertFalse("Should return configs", results.head.configs().isEmpty)
   }
+
+  @Test
+  def testDescribeConfigsWithDocumentation(): Unit = {
+    EasyMock.expect(zkClient.getEntityConfigs(ConfigType.Topic, topic)).andReturn(new java.util.Properties)
+    EasyMock.expect(zkClient.getEntityConfigs(ConfigType.Broker, brokerId.toString)).andReturn(TestUtils.createBrokerConfig(brokerId,"zk"))
+    EasyMock.expect(metadataCache.contains(topic)).andReturn(true)
+    EasyMock.replay(zkClient, metadataCache)
+
+    val adminManager = createAdminManager()
+
+    val resources = List(
+      new DescribeConfigsRequestData.DescribeConfigsResource()
+        .setResourceName(topic)
+        .setResourceType(ConfigResource.Type.TOPIC.id),
+      new DescribeConfigsRequestData.DescribeConfigsResource()
+        .setResourceName(brokerId.toString)
+        .setResourceType(ConfigResource.Type.BROKER.id))
+
+    val results: List[DescribeConfigsResponseData.DescribeConfigsResult] = adminManager.describeConfigs(resources, true, true)
+    assertEquals(2, results.size)
+    results.foreach(r => {
+      assertEquals(Errors.NONE.code, r.errorCode)
+      assertFalse("Should return configs", r.configs.isEmpty)
+      r.configs.forEach(c => assertNotNull(s"Config ${c.name} should have documentation", c.documentation))

Review comment:
       we could check documentation is not empty as well?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [kafka] mimaison merged pull request #9360: KAFKA-10557: Missing docs when describing topic configs including

Posted by GitBox <gi...@apache.org>.
mimaison merged pull request #9360:
URL: https://github.com/apache/kafka/pull/9360


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org