KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand by showuon · Pull Request #10471 · apache/kafka

showuon · 2021-04-04T03:13:53Z

remove deprecated --zookeeper option in ReassignPartitionsCommand. And also the zookeeper dependent methods, and the tests.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…Command

showuon · 2021-04-04T06:58:14Z

                      .withRequiredArg
                      .describedAs("Server(s) to use for bootstrapping")
                      .ofType(classOf[String])
+                      .required()


Make bootstrapServerOpt as required

showuon · 2021-04-04T07:01:03Z

@ijuma , could you help review this PR? Thanks.

ijuma

Thanks for the PR. A few comments below.

ijuma · 2021-04-06T12:52:48Z

    var failed = true

+    val props = if (opts.options.has(opts.commandConfigOpt))
+      Utils.loadProps(opts.options.valueOf(opts.commandConfigOpt))


Why was this moved outside the try?

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.

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

ijuma · 2021-04-06T12:53:58Z

        println(Utils.stackTrace(e))
    } finally {
-      // Close the AdminClient or ZooKeeper client, as appropriate.
+      // Close the AdminClient, as appropriate.


This comment is redundant IMO

ijuma · 2021-04-06T12:56:47Z

@@ -1695,14 +1367,12 @@ object ReassignPartitionsCommand extends Logging {
        opts.bootstrapServerOpt,


Do we need to pass bootstrapServerOpt here and the lines below?

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

ijuma · 2021-04-06T13:04:13Z

+  def shouldPrintHelpTextIfHelpArg(): Unit = {
+    val args: Array[String]= Array(
+      "--help",
+      "--bootstrap-server", "localhost:1234")


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

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: #10457 on the ConfigCommand tomorrow. Thank you.

Makes sense.

showuon · 2021-04-07T09:36:41Z

@ijuma , thanks for the comments. I've updated with:

move Admin.create into try block
check --bootstrap-server option manually, so that help text can be displayed when no option provided.

Thanks.

showuon · 2021-04-16T13:31:11Z

@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

showuon · 2021-05-18T02:16:46Z

@ijuma , could you take a look again for this PR? Thanks.

ijuma

Thanks for the updates, a couple of comments.

ijuma · 2021-05-19T12:42:09Z

    var failed = true

+    val props = if (opts.options.has(opts.commandConfigOpt))
+      Utils.loadProps(opts.options.valueOf(opts.commandConfigOpt))


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

ijuma · 2021-05-19T12:46:38Z

-    shouldFailWith("You must specify --bootstrap-server when using \"[command-config]\"", args)
+  def shouldPrintHelpTextIfHelpArg(): Unit = {
+    val args: Array[String]= Array("--help")
+    shouldFailWith(ReassignPartitionsCommand.helpText, args)


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

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

showuon · 2021-05-20T07:56:12Z

@ijuma , thanks for the comments. I've updated. Please take a look again. Thank you.

showuon · 2021-06-01T12:04:41Z

@ijuma , could you help check this PR again? Thanks.

showuon · 2021-06-01T12:05:56Z

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

ijuma

Thanks for the PR. I left a couple of comments below. In addition, we should add a note to upgrade.html for this removal as well as the topic command one.

ijuma · 2021-06-01T14:13:36Z

        PartitionReassignmentState(Seq(3, 2, 0), Seq(3, 2, 0), true)
    )

-    // When using --zookeeper, we aren't able to see the new-style assignment


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

This comment is kind of weird here, but as here 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.

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

showuon · 2021-06-03T07:55:45Z

@ijuma , thanks for your comments. I've addressed them and add a note in upgrade.html in this commit: 9215777. For the system tests, I've created another jira ticket to track it: KAFKA-12884. Thank you.

ijuma

LGTM, just one nit below.

ijuma · 2021-06-03T13:23:35Z

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


"command line tool" -> "command line tools"

Updated. Thanks.

ijuma · 2021-06-03T13:29:22Z

        PartitionReassignmentState(Seq(3, 2, 0), Seq(3, 2, 0), true)
    )

-    // When using --zookeeper, we aren't able to see the new-style assignment


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

ijuma · 2021-06-04T06:29:45Z

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

showuon · 2021-06-04T06:33:26Z

Thanks @ijuma , I should have listed all failed tests and verify they are all un-related failure to reduce your review task. Thank you. :)

…nmentWithAlterIsrDisabled (#11244) Removes assertion added in #10471. It's unsafe to assert that there are partition movements ongoing for some of the tests in the suite because partitions in some of the tests have 0 data, which may complete reassignment before `verify` can run. Tests pass locally. Reviewers: Luke Chen <showuon@gmail.com>, Ismael Juma <ismael@juma.me.uk>

…nmentWithAlterIsrDisabled (apache#11244) Removes assertion added in apache#10471. It's unsafe to assert that there are partition movements ongoing for some of the tests in the suite because partitions in some of the tests have 0 data, which may complete reassignment before `verify` can run. Tests pass locally. Reviewers: Luke Chen <showuon@gmail.com>, Ismael Juma <ismael@juma.me.uk>

KAFKA-12597: remove deprecated zookeeper option in ReassignPartitions…

cafec30

…Command

showuon force-pushed the KAFKA-12597 branch from 266d9ef to cafec30 Compare April 4, 2021 06:49

showuon commented Apr 4, 2021

View reviewed changes

ijuma reviewed Apr 6, 2021

View reviewed changes

showuon force-pushed the KAFKA-12597 branch 3 times, most recently from c3666e7 to b95c713 Compare April 7, 2021 09:34

KAFKA-12597: check --bootstrap-server option manually

d61a24e

showuon force-pushed the KAFKA-12597 branch from b95c713 to d61a24e Compare April 7, 2021 09:52

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-12597

c68338b

ijuma mentioned this pull request May 19, 2021

KAFKA-12783: Remove the deprecated ZK-based partition reassignment API #10695

Closed

ijuma reviewed May 19, 2021

View reviewed changes

showuon added 2 commits May 20, 2021 14:28

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-12597

adecdb2

KAFKA-12597: put loadProps into try block

d48e215

ijuma mentioned this pull request Jun 1, 2021

KAFKA-12596: remove --zookeeper option from topic command #10457

Merged

3 tasks

ijuma reviewed Jun 1, 2021

View reviewed changes

showuon added 2 commits June 3, 2021 11:44

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-12597

2e8a245

KAFKA-12597: refactor tests

9215777

ijuma approved these changes Jun 3, 2021

View reviewed changes

KAFKA-12597: update typo

8dc4731

ijuma merged commit ccde334 into apache:trunk Jun 4, 2021

splett2 mentioned this pull request Aug 20, 2021

KAFKA-12933: Flaky test ReassignPartitionsIntegrationTest.testReassignmentWithAlterIsrDisabled #11244

Merged

3 tasks

		@@ -1695,14 +1367,12 @@ object ReassignPartitionsCommand extends Logging {
		opts.bootstrapServerOpt,

Conversation

showuon commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon commented Apr 4, 2021

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon commented Apr 7, 2021

Uh oh!

showuon commented Apr 16, 2021

Uh oh!

showuon commented May 18, 2021

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon commented May 20, 2021

Uh oh!

showuon commented Jun 1, 2021

Uh oh!

showuon commented Jun 1, 2021

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon commented Jun 3, 2021

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

showuon commented Jun 4, 2021

Uh oh!

Reviewers

showuon commented Apr 4, 2021 •

edited

Loading

showuon Jun 3, 2021 •

edited

Loading

ijuma commented Jun 4, 2021 •

edited

Loading