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 2021/04/04 03:14:05 UTC

[GitHub] [kafka] showuon opened a new pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   
   
   ### 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] showuon commented on a change in pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala
##########
@@ -1775,29 +1429,26 @@ object ReassignPartitionsCommand extends Logging {
 
   sealed class ReassignPartitionsCommandOptions(args: Array[String]) extends CommandDefaultOptions(args)  {
     // Actions
-    val verifyOpt = parser.accepts("verify", "Verify if the reassignment completed as specified by the --reassignment-json-file option. If there is a throttle engaged for the replicas specified, and the rebalance has completed, the throttle will be removed")
+    val verifyOpt = parser.accepts("verify", "Verify if the reassignment completed as specified by the " +
+      "--reassignment-json-file option. If there is a throttle engaged for the replicas specified, and the rebalance has completed, the throttle will be removed")
     val generateOpt = parser.accepts("generate", "Generate a candidate partition reassignment configuration." +
       " Note that this only generates a candidate assignment, it does not execute it.")
     val executeOpt = parser.accepts("execute", "Kick off the reassignment as specified by the --reassignment-json-file option.")
     val cancelOpt = parser.accepts("cancel", "Cancel an active reassignment.")
     val listOpt = parser.accepts("list", "List all active partition reassignments.")
 
     // Arguments
-    val bootstrapServerOpt = parser.accepts("bootstrap-server", "the server(s) to use for bootstrapping. REQUIRED if " +
-                      "an absolute path of the log directory is specified for any replica in the reassignment json file, " +
-                      "or if --zookeeper is not given.")
+    val bootstrapServerOpt = parser.accepts("bootstrap-server", "REQUIRED: the server(s) to use for bootstrapping.")
                       .withRequiredArg
                       .describedAs("Server(s) to use for bootstrapping")
                       .ofType(classOf[String])
+                      .required()

Review comment:
       Make `bootstrapServerOpt` as required




-- 
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] ijuma commented on pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   Unrelated test failures:
   
   > Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic() | 16 sec | 1
   > -- | -- | --
   > Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic() | 24 sec | 1
   > Build / JDK 8 and Scala 2.12 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopicsWithManyPartitions()
   


-- 
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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -128,11 +126,10 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
         PartitionReassignmentState(Seq(3, 2, 0), Seq(3, 2, 0), true)
     )
 
-    // When using --zookeeper, we aren't able to see the new-style assignment

Review comment:
       This comment is kind of weird here, but as [here](https://github.com/apache/kafka/pull/10471/files#diff-7282fb89afcc2ee0a045cc7f5c182691a9279f85d8a9c178471253c13953f6bdL366) mentioned:
   
   > Warning: because you are using the deprecated --zookeeper option, the results may be incomplete.  Use --bootstrap-server instead for more accurate results.
   
   So, I added one more verification here due to we change to adminClient now. The most important finalAssignment is already verified. 
   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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -151,7 +148,7 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
       """{"topic":"bar","partition":0,"replicas":[3,2,0],"log_dirs":["any","any","any"]}""" +
       """]}"""
     // Execute the assignment
-    runExecuteAssignment(zkClient, assignment, -1L)
+    runExecuteAssignment(cluster.adminClient, false, assignment, -1L, -1L)

Review comment:
       Nice catch! The test should also be removed since there's already a test `testReassignment` to test adminClient reassignment. Thank you.

##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -128,11 +126,10 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
         PartitionReassignmentState(Seq(3, 2, 0), Seq(3, 2, 0), true)
     )
 
-    // When using --zookeeper, we aren't able to see the new-style assignment

Review comment:
       This comment is kind of weird here, but as [here](https://github.com/apache/kafka/pull/10471/files#diff-7282fb89afcc2ee0a045cc7f5c182691a9279f85d8a9c178471253c13953f6bdL366) mentioned:
   
   > Warning: because you are using the deprecated --zookeeper option, the results may be incomplete.  Use --bootstrap-server instead for more accurate results.
   
   So, I add one more verification here due to we change to adminClient now.
   Thank you.

##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -128,11 +126,10 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
         PartitionReassignmentState(Seq(3, 2, 0), Seq(3, 2, 0), true)
     )
 
-    // When using --zookeeper, we aren't able to see the new-style assignment

Review comment:
       This comment is kind of weird here, but as [here](https://github.com/apache/kafka/pull/10471/files#diff-7282fb89afcc2ee0a045cc7f5c182691a9279f85d8a9c178471253c13953f6bdL366) mentioned:
   
   > Warning: because you are using the deprecated --zookeeper option, the results may be incomplete.  Use --bootstrap-server instead for more accurate results.
   
   So, I added one more verification here due to we change to adminClient now. The most important finalAssignment will be verified next. 
   Thank you.

##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -128,11 +126,10 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
         PartitionReassignmentState(Seq(3, 2, 0), Seq(3, 2, 0), true)
     )
 
-    // When using --zookeeper, we aren't able to see the new-style assignment

Review comment:
       This comment is kind of weird here, but as [here](https://github.com/apache/kafka/pull/10471/files#diff-7282fb89afcc2ee0a045cc7f5c182691a9279f85d8a9c178471253c13953f6bdL366) mentioned:
   
   > Warning: because you are using the deprecated --zookeeper option, the results may be incomplete.  Use --bootstrap-server instead for more accurate results.
   
   So, I added one more verification here due to we change to adminClient now. The most important finalAssignment is already verified. 
   Thank you.

##########
File path: docs/upgrade.html
##########
@@ -43,7 +43,8 @@ <h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3
             <code>AclBindingFilter</code>.</li>
         <li>The <code>Admin.electedPreferredLeaders()</code> methods were removed. Please use <code>Admin.electLeaders</code> instead.</li>
         <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> command line tool. Please use <code>--bootstrap-server</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 tool.

Review comment:
       Updated. 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] ijuma commented on a change in pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsCommandArgsTest.scala
##########
@@ -230,15 +214,11 @@ class ReassignPartitionsCommandArgsTest {
   }
 
   @Test
-  def testInvalidCommandConfigArgumentForLegacyGenerate(): Unit = {
-    val args = Array(
-      "--zookeeper", "localhost:1234",
-      "--generate",
-      "--broker-list", "101,102",
-      "--topics-to-move-json-file", "myfile.json",
-      "--command-config", "/tmp/command-config.properties"
-    )
-    shouldFailWith("You must specify --bootstrap-server when using \"[command-config]\"", args)
+  def shouldPrintHelpTextIfHelpArg(): Unit = {
+    val args: Array[String]= Array(
+      "--help",
+      "--bootstrap-server", "localhost:1234")

Review comment:
       Makes sense.




-- 
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] ijuma merged pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   


-- 
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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsCommandArgsTest.scala
##########
@@ -230,15 +214,9 @@ class ReassignPartitionsCommandArgsTest {
   }
 
   @Test
-  def testInvalidCommandConfigArgumentForLegacyGenerate(): Unit = {
-    val args = Array(
-      "--zookeeper", "localhost:1234",
-      "--generate",
-      "--broker-list", "101,102",
-      "--topics-to-move-json-file", "myfile.json",
-      "--command-config", "/tmp/command-config.properties"
-    )
-    shouldFailWith("You must specify --bootstrap-server when using \"[command-config]\"", args)
+  def shouldPrintHelpTextIfHelpArg(): Unit = {
+    val args: Array[String]= Array("--help")
+    shouldFailWith(ReassignPartitionsCommand.helpText, args)

Review comment:
       Yes, it should not fail actually. But we share the same `printUsageAndDie` method with other wrong argument cases, so it indeed fail in the logic. I think it's OK since user won't feel it's a failed run anyway. I'll add a comment here. Thanks.
   
   ref: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/utils/CommandLineUtils.scala#L57




-- 
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] ijuma commented on pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   Unrelated test failures:
   
   > Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic() | 16 sec | 1
   > -- | -- | --
   > Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic() | 24 sec | 1
   > Build / JDK 8 and Scala 2.12 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopicsWithManyPartitions()
   


-- 
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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala
##########
@@ -194,33 +192,18 @@ object ReassignPartitionsCommand extends Logging {
 
   def main(args: Array[String]): Unit = {
     val opts = validateAndParseArgs(args)
-    var toClose: Option[AutoCloseable] = None
     var failed = true
 
+    val props = if (opts.options.has(opts.commandConfigOpt))
+      Utils.loadProps(opts.options.valueOf(opts.commandConfigOpt))

Review comment:
       Good question. The lines I moved out of `try` block are props setup (which is safe), and `Admin.create`. I was trying to do try-with-resource in scala, but it can't. I moved the `Admin.create` back to try block. 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 pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   @ijuma , thanks for your comments. I've addressed them and add a note in upgrade.html in this commit: https://github.com/apache/kafka/pull/10471/commits/92157779ce7fa347337916b01c88f9cc415165fe. For the system  tests, I've created another jira ticket to track it: KAFKA-12884. 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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsCommandArgsTest.scala
##########
@@ -230,15 +214,11 @@ class ReassignPartitionsCommandArgsTest {
   }
 
   @Test
-  def testInvalidCommandConfigArgumentForLegacyGenerate(): Unit = {
-    val args = Array(
-      "--zookeeper", "localhost:1234",
-      "--generate",
-      "--broker-list", "101,102",
-      "--topics-to-move-json-file", "myfile.json",
-      "--command-config", "/tmp/command-config.properties"
-    )
-    shouldFailWith("You must specify --bootstrap-server when using \"[command-config]\"", args)
+  def shouldPrintHelpTextIfHelpArg(): Unit = {
+    val args: Array[String]= Array(
+      "--help",
+      "--bootstrap-server", "localhost:1234")

Review comment:
       Well, I agree. Making the `--bootstrap-server` as required argument caused this side effect. I think we should keep the `--bootstrap-server` as non-required, and do the argument check by ourselves. I'll update it, and also the other PR: https://github.com/apache/kafka/pull/10457 on the `ConfigCommand` tomorrow. 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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -128,11 +126,10 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
         PartitionReassignmentState(Seq(3, 2, 0), Seq(3, 2, 0), true)
     )
 
-    // When using --zookeeper, we aren't able to see the new-style assignment

Review comment:
       This comment is kind of weird here, but as [here](https://github.com/apache/kafka/pull/10471/files#diff-7282fb89afcc2ee0a045cc7f5c182691a9279f85d8a9c178471253c13953f6bdL366) mentioned:
   
   > Warning: because you are using the deprecated --zookeeper option, the results may be incomplete.  Use --bootstrap-server instead for more accurate results.
   
   So, I add one more verification here due to we change to adminClient now.
   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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -128,11 +126,10 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
         PartitionReassignmentState(Seq(3, 2, 0), Seq(3, 2, 0), true)
     )
 
-    // When using --zookeeper, we aren't able to see the new-style assignment

Review comment:
       This comment is kind of weird here, but as [here](https://github.com/apache/kafka/pull/10471/files#diff-7282fb89afcc2ee0a045cc7f5c182691a9279f85d8a9c178471253c13953f6bdL366) mentioned:
   
   > Warning: because you are using the deprecated --zookeeper option, the results may be incomplete.  Use --bootstrap-server instead for more accurate results.
   
   So, I added one more verification here due to we change to adminClient now. The most important finalAssignment will be verified next. 
   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] ijuma commented on a change in pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -128,11 +126,10 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
         PartitionReassignmentState(Seq(3, 2, 0), Seq(3, 2, 0), true)
     )
 
-    // When using --zookeeper, we aren't able to see the new-style assignment

Review comment:
       Yeah, I don't know what it means by new-style assignment either.

##########
File path: docs/upgrade.html
##########
@@ -43,7 +43,8 @@ <h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3
             <code>AclBindingFilter</code>.</li>
         <li>The <code>Admin.electedPreferredLeaders()</code> methods were removed. Please use <code>Admin.electLeaders</code> instead.</li>
         <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> command line tool. Please use <code>--bootstrap-server</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 tool.

Review comment:
       "command line tool" -> "command line tools"




-- 
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] ijuma commented on a change in pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala
##########
@@ -194,33 +192,18 @@ object ReassignPartitionsCommand extends Logging {
 
   def main(args: Array[String]): Unit = {
     val opts = validateAndParseArgs(args)
-    var toClose: Option[AutoCloseable] = None
     var failed = true
 
+    val props = if (opts.options.has(opts.commandConfigOpt))
+      Utils.loadProps(opts.options.valueOf(opts.commandConfigOpt))

Review comment:
       Why was this moved outside the `try`?

##########
File path: core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala
##########
@@ -229,9 +212,9 @@ object ReassignPartitionsCommand extends Logging {
         println("Error: " + e.getMessage)
         println(Utils.stackTrace(e))
     } finally {
-      // Close the AdminClient or ZooKeeper client, as appropriate.
+      // Close the AdminClient, as appropriate.

Review comment:
       This comment is redundant IMO

##########
File path: core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala
##########
@@ -1695,14 +1367,12 @@ object ReassignPartitionsCommand extends Logging {
         opts.bootstrapServerOpt,

Review comment:
       Do we need to pass `bootstrapServerOpt` here and the lines below?

##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsCommandArgsTest.scala
##########
@@ -230,15 +214,11 @@ class ReassignPartitionsCommandArgsTest {
   }
 
   @Test
-  def testInvalidCommandConfigArgumentForLegacyGenerate(): Unit = {
-    val args = Array(
-      "--zookeeper", "localhost:1234",
-      "--generate",
-      "--broker-list", "101,102",
-      "--topics-to-move-json-file", "myfile.json",
-      "--command-config", "/tmp/command-config.properties"
-    )
-    shouldFailWith("You must specify --bootstrap-server when using \"[command-config]\"", args)
+  def shouldPrintHelpTextIfHelpArg(): Unit = {
+    val args: Array[String]= Array(
+      "--help",
+      "--bootstrap-server", "localhost:1234")

Review comment:
       Hmm, we should not require this line to see the help text.




-- 
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] ijuma edited a comment on pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

Posted by GitBox <gi...@apache.org>.
ijuma edited a comment on pull request #10471:
URL: https://github.com/apache/kafka/pull/10471#issuecomment-854399316


   Unrelated test failures:
   
   > Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic()
   > Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic()
   > Build / JDK 8 and Scala 2.12 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopicsWithManyPartitions()
   


-- 
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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   @ijuma , could you help check this PR again? 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] showuon commented on pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   Failed tests are flaky `RaftClusterTest` tests. Thanks.
   ```
       Build / JDK 15 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()
       Build / JDK 15 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic()
   ```


-- 
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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   Thanks @ijuma , I should have listed all failed tests and verify they are all un-related failure to reduce your review task. 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 pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   @ijuma , could you take a look again for this PR? 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] ijuma commented on a change in pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -151,7 +148,7 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
       """{"topic":"bar","partition":0,"replicas":[3,2,0],"log_dirs":["any","any","any"]}""" +
       """]}"""
     // Execute the assignment
-    runExecuteAssignment(zkClient, assignment, -1L)
+    runExecuteAssignment(cluster.adminClient, false, assignment, -1L, -1L)

Review comment:
       Looks like the test name doesn't match what we're testing now.

##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -128,11 +126,10 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
         PartitionReassignmentState(Seq(3, 2, 0), Seq(3, 2, 0), true)
     )
 
-    // When using --zookeeper, we aren't able to see the new-style assignment

Review comment:
       Since we are removing this comment, should we be using new style assignment?




-- 
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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   @ijuma , could you please help review the PR? Thank you. 
   The failed tests are all un-related.
   ```
   
       Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testOneWayReplicationWithAutoOffsetSync()
       Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testReplicationWithEmptyPartition()
       Build / JDK 11 and Scala 2.13 / kafka.controller.ControllerIntegrationTest.testPartitionReassignmentToBrokerWithOfflineLogDir()
       Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()
       Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()
       Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testOneWayReplicationWithAutoOffsetSync()
       Build / JDK 15 and Scala 2.13 / kafka.log.LogCleanerIntegrationTest.testMarksPartitionsAsOfflineAndPopulatesUncleanableMetrics()
       Build / JDK 15 and Scala 2.13 / kafka.network.SocketServerTest.testUnmuteChannelWithBufferedReceives()
       Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplication()
       Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplication()
       Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testOneWayReplicationWithAutoOffsetSync()
       Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testOneWayReplicationWithAutoOffsetSync
   
   ```


-- 
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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   @ijuma , could you help review this PR? 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] showuon commented on a change in pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -151,7 +148,7 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
       """{"topic":"bar","partition":0,"replicas":[3,2,0],"log_dirs":["any","any","any"]}""" +
       """]}"""
     // Execute the assignment
-    runExecuteAssignment(zkClient, assignment, -1L)
+    runExecuteAssignment(cluster.adminClient, false, assignment, -1L, -1L)

Review comment:
       Nice catch! The test should also be removed since there's already a test `testReassignment` to test adminClient reassignment. 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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: docs/upgrade.html
##########
@@ -43,7 +43,8 @@ <h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3
             <code>AclBindingFilter</code>.</li>
         <li>The <code>Admin.electedPreferredLeaders()</code> methods were removed. Please use <code>Admin.electLeaders</code> instead.</li>
         <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> command line tool. Please use <code>--bootstrap-server</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 tool.

Review comment:
       Updated. 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] showuon commented on pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   @ijuma , thanks for the comments. I've updated. Please take a look again. 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 pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   @ijuma , thanks for the comments. I've updated with:
   1. move `Admin.create` into try block
   2. check `--bootstrap-server` option manually, so that help text can be displayed when no option provided.
   
   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] showuon commented on a change in pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala
##########
@@ -1695,14 +1367,12 @@ object ReassignPartitionsCommand extends Logging {
         opts.bootstrapServerOpt,

Review comment:
       Yes, if `bootstrapServerOpt` is not in the permitted arg list, the check will fail since `bootstrapServerOpt` is not permitted option for this action.




-- 
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] ijuma edited a comment on pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

Posted by GitBox <gi...@apache.org>.
ijuma edited a comment on pull request #10471:
URL: https://github.com/apache/kafka/pull/10471#issuecomment-854399316


   Unrelated test failures:
   
   > Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic()
   > Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic()
   > Build / JDK 8 and Scala 2.12 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopicsWithManyPartitions()
   


-- 
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] ijuma commented on a change in pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala
##########
@@ -128,11 +126,10 @@ class ReassignPartitionsIntegrationTest extends ZooKeeperTestHarness {
         PartitionReassignmentState(Seq(3, 2, 0), Seq(3, 2, 0), true)
     )
 
-    // When using --zookeeper, we aren't able to see the new-style assignment

Review comment:
       Yeah, I don't know what it means by new-style assignment either.

##########
File path: docs/upgrade.html
##########
@@ -43,7 +43,8 @@ <h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3
             <code>AclBindingFilter</code>.</li>
         <li>The <code>Admin.electedPreferredLeaders()</code> methods were removed. Please use <code>Admin.electLeaders</code> instead.</li>
         <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> command line tool. Please use <code>--bootstrap-server</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 tool.

Review comment:
       "command line tool" -> "command line tools"




-- 
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] ijuma merged pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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


   


-- 
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] ijuma commented on a change in pull request #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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



##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsCommandArgsTest.scala
##########
@@ -230,15 +214,9 @@ class ReassignPartitionsCommandArgsTest {
   }
 
   @Test
-  def testInvalidCommandConfigArgumentForLegacyGenerate(): Unit = {
-    val args = Array(
-      "--zookeeper", "localhost:1234",
-      "--generate",
-      "--broker-list", "101,102",
-      "--topics-to-move-json-file", "myfile.json",
-      "--command-config", "/tmp/command-config.properties"
-    )
-    shouldFailWith("You must specify --bootstrap-server when using \"[command-config]\"", args)
+  def shouldPrintHelpTextIfHelpArg(): Unit = {
+    val args: Array[String]= Array("--help")
+    shouldFailWith(ReassignPartitionsCommand.helpText, args)

Review comment:
       Should this actually fail? It seems that it should succeed, right?

##########
File path: core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala
##########
@@ -194,33 +192,18 @@ object ReassignPartitionsCommand extends Logging {
 
   def main(args: Array[String]): Unit = {
     val opts = validateAndParseArgs(args)
-    var toClose: Option[AutoCloseable] = None
     var failed = true
 
+    val props = if (opts.options.has(opts.commandConfigOpt))
+      Utils.loadProps(opts.options.valueOf(opts.commandConfigOpt))

Review comment:
       Why is props setup safe? It could fail to load the fail, etc.




-- 
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 #10471: KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand

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






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