[GitHub] [kafka] ijuma commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config

[GitBox <gi...@apache.org>]

ijuma commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r659800702



##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -68,7 +68,7 @@ object ConfigCommand extends Config {
   val BrokerDefaultEntityName = ""
   val BrokerLoggerConfigType = "broker-loggers"
   val BrokerSupportedConfigTypes = ConfigType.all :+ BrokerLoggerConfigType
-  val ZkSupportedConfigTypes = ConfigType.all
+  val ZkSupportedConfigTypes = Seq(ConfigType.User, ConfigType.Broker)

Review comment:
       Wouldn't this allow quota updates via --zookeeper?

##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,30 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
       "--add-config", "security.inter.broker.protocol=PLAINTEXT"))
   }
 
+  @Test
+  def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+    assertNonZeroStatusExit(Array(
+      "--zookeeper", zkConnect,
+      "--entity-type", "topics",
+      "--describe"))
+  }
+
+  @Test
+  def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+    assertNonZeroStatusExit(Array(
+      "--zookeeper", zkConnect,
+      "--entity-type", "clients",
+      "--describe"))
+  }
+
+  @Test
+  def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+    assertNonZeroStatusExit(Array(
+      "--zookeeper", zkConnect,
+      "--entity-type", "Ips",

Review comment:
       Is "Ips" correct?

##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,30 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
       "--add-config", "security.inter.broker.protocol=PLAINTEXT"))
   }
 
+  @Test
+  def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+    assertNonZeroStatusExit(Array(
+      "--zookeeper", zkConnect,
+      "--entity-type", "topics",
+      "--describe"))
+  }
+
+  @Test
+  def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+    assertNonZeroStatusExit(Array(
+      "--zookeeper", zkConnect,
+      "--entity-type", "clients",

Review comment:
       We should have a test like this but for `user` quotas.

##########
File path: docs/upgrade.html
##########
@@ -47,6 +47,8 @@ <h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3
         <li>The <code>kafka-preferred-replica-election</code> command line tool was removed. Please use <code>kafka-leader-election</code> instead.</li>
         <li>The <code>--zookeeper</code> option was removed from the <code>bin/kafka-topics.sh</code> and <code>bin/kafka-reassign-partitions.sh</code> command line tools.
             Please use <code>--bootstrap-server</code> instead.</li>
+        <li>In <code>bin/kafka-configs.sh</code>, the <code>--zookeeper</code> option only supported for <a href="#security_sasl_scram_credentials">SCRAM Credentials configuration</a>

Review comment:
       I would follow a similar pattern as the other lines. For example, "The <code>kafka-configs</code> command..."




-- 
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-unsubscribe@kafka.apache.org

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