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/05/15 14:03:31 UTC

[GitHub] [kafka] showuon opened a new pull request #8675: KAFKA-10004: Fix the default broker configs cannot be displayed when using kafka-configs.sh --describe

showuon opened a new pull request #8675:
URL: https://github.com/apache/kafka/pull/8675


   When running
   `bin/kafka-configs.sh --describe --bootstrap-server localhost:9092 --entity-type brokers`
   the output will be:
   Dynamic configs for broker 0 are: ....
   Dynamic configs for broker <default> are:
   **The entity name for brokers must be a valid integer broker id, found: <default>**
   
   I found it's because we set the default entity name for broker as `<default>`, which should be empty string "". Also add the missing tests for the describe broker without entity name case.
   
   ### 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] cmccabe commented on pull request #8675: KAFKA-10004: ConfigCommand fails to find default broker configs without ZK

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


   Backported to 2.5.1


----------------------------------------------------------------
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] cmccabe merged pull request #8675: KAFKA-10004: Fix the default broker configs cannot be displayed when using kafka-configs.sh --describe

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


   


----------------------------------------------------------------
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] showuon commented on pull request #8675: KAFKA-10004: Fix the default broker configs cannot be displayed when using kafka-configs.sh --describe

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


   @bdbyrne @stanislavkozlovski @cmccabe , would you please review this PR when available? Thank you.


----------------------------------------------------------------
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] showuon commented on a change in pull request #8675: KAFKA-10004: Fix the default broker configs cannot be displayed when using kafka-configs.sh --describe

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



##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -424,7 +425,7 @@ object ConfigCommand extends Config {
         case ConfigType.Topic =>
           adminClient.listTopics(new ListTopicsOptions().listInternal(true)).names().get().asScala.toSeq
         case ConfigType.Broker | BrokerLoggerConfigType =>
-          adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+ ConfigEntityName.Default
+          adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+ BrokerDefaultEntityName

Review comment:
       Thanks @bdbyrne . Good suggestion. I've updated it in this commit https://github.com/apache/kafka/pull/8675/commits/83cc2328099ef1424c3cea492156bd4a82f3fe65. Thank you.




----------------------------------------------------------------
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] showuon commented on a change in pull request #8675: KAFKA-10004: Fix the default broker configs cannot be displayed when using kafka-configs.sh --describe

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



##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -736,6 +731,12 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
     verifyAlterBrokerConfig(node, "", List("--entity-default"))
   }
 
+  @Test
+  def shouldAddBrokerDynamicConfig(): Unit = {
+    val node = new Node(1, "localhost", 9092)
+    verifyAlterBrokerConfig(node, "1", List("--entity-name", "1"))
+  }

Review comment:
       reorder this test case to put the `verifyAlterBrokerConfig` test suits together. Note: the previous test is also test with `verifyAlterBrokerConfig`




----------------------------------------------------------------
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] cmccabe commented on pull request #8675: KAFKA-10004: Fix the default broker configs cannot be displayed when using kafka-configs.sh --describe

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


   @showuon : thanks for the fix!  You might want to fix your git configuration since your email  is showing up as `showuon <43...@users.noreply.github.com>`


----------------------------------------------------------------
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] showuon commented on a change in pull request #8675: KAFKA-10004: Fix the default broker configs cannot be displayed when using kafka-configs.sh --describe

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



##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -646,7 +641,7 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
 
   @Test
   def testNoSpecifiedEntityOptionWithDescribeBrokersInZKIsAllowed(): Unit = {
-    val optsList = List("--zookeeper", "localhost:9092",
+    val optsList = List("--zookeeper", zkConnect,

Review comment:
       fix the wrong zk connection info




----------------------------------------------------------------
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] bdbyrne commented on a change in pull request #8675: KAFKA-10004: Fix the default broker configs cannot be displayed when using kafka-configs.sh --describe

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



##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -424,7 +425,7 @@ object ConfigCommand extends Config {
         case ConfigType.Topic =>
           adminClient.listTopics(new ListTopicsOptions().listInternal(true)).names().get().asScala.toSeq
         case ConfigType.Broker | BrokerLoggerConfigType =>
-          adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+ ConfigEntityName.Default
+          adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+ BrokerDefaultEntityName

Review comment:
       Please substitute `""` for `BrokerDefaultEntityName` in the `match` below, as well as in `getResourceConfig`.




----------------------------------------------------------------
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] cmccabe commented on pull request #8675: KAFKA-10004: Fix the default broker configs cannot be displayed when using kafka-configs.sh --describe

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


   ok to test


----------------------------------------------------------------
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] cmccabe commented on pull request #8675: KAFKA-10004: Fix the default broker configs cannot be displayed when using kafka-configs.sh --describe

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


   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.

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