KAFKA-12596: remove --zookeeper option from topic command by showuon · Pull Request #10457 · apache/kafka

showuon · 2021-04-01T09:24:11Z

Remove ZookeeperTopicService and the test using zookeeper TopicCommandWithZKClientTest

Committer Checklist (excluded from commit message)

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

showuon · 2021-04-01T09:25:46Z

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

ijuma

Thanks for the PR. Maybe we can remove the TopicService trait and rename AdminClientTopicService to TopicService? Do we use the trait for anything now that the ZK code was removed?

ijuma · 2021-04-01T12:09:07Z

+      "if set when describing topics, only show partitions whose isr count is less than the configured minimum.")
    private val reportAtMinIsrPartitionsOpt = parser.accepts("at-min-isr-partitions",
-      "if set when describing topics, only show partitions whose isr count is equal to the configured minimum. Not supported with the --zookeeper option.")
+      "if set when describing topics, only show partitions whose isr count is equal to the configured minimum.")


We have this line:

// This is not currently used, but we keep it for compatibility parser.accepts("force", "Suppress console prompts")

Was this deprecated also? Can we remove it?

I confirmed this issue is fixed in KIP-74 (KAFKA-2063). Yes I'll remove it!. Thank you.

ijuma · 2021-04-01T12:09:39Z

-
      if (!has(bootstrapServerOpt))
-        CommandLineUtils.checkRequiredArgs(parser, options, zkConnectOpt)
+        throw new IllegalArgumentException("--bootstrap-server must be specified")


I think you can mark the argument as required to get this behavior automatically.

Good suggestion! Updated

ijuma · 2021-04-01T12:10:35Z

-      if (has(createOpt) && !has(replicaAssignmentOpt) && has(zkConnectOpt))
+      if (has(createOpt) && !has(replicaAssignmentOpt))
        CommandLineUtils.checkRequiredArgs(parser, options, partitionsOpt, replicationFactorOpt)
      if (has(bootstrapServerOpt) && has(alterOpt)) {


We don't need the has(bootstrapServerOpt) check.

You're right.

showuon · 2021-04-02T09:37:12Z

  }

-  case class AdminClientTopicService private (adminClient: Admin) extends TopicService {
+  case class TopicService private (adminClient: Admin) extends AutoCloseable {


Rename AdminClientTopicService into TopicService

This should not be a case class.

showuon · 2021-04-02T09:37:32Z

    }
  }

-  trait TopicService extends AutoCloseable {


remove TopicService trait since we only have one service now.

showuon · 2021-04-02T09:38:12Z

      .withRequiredArg
      .describedAs("server to connect to")
      .ofType(classOf[String])
+      .required()


Set as required argument.

showuon · 2021-04-02T09:40:34Z

  def testCreate(): Unit = {
    createAndWaitTopic(new TopicCommandOptions(
-      Array("--partitions", "2", "--replication-factor", "1", "--topic", testTopicName)))
+      brokerOptions ++ Array("--partitions", "2", "--replication-factor", "1", "--topic", testTopicName)))


We didn't checkArgs before, so it's ok to not pass the --bootstrap-server option. Since we make it as required argument now, we need to pass the option explicitly.

showuon · 2021-04-02T09:42:07Z

@ijuma , thanks for the good suggestion. Yes, we don't need the TopicService trait now. Updated. Please help review it again. Thank you.

ijuma · 2021-04-02T19:33:41Z

@@ -65,9 +65,10 @@ class TopicCommandWithAdminClientTest extends KafkaServerTestHarness with Loggin
  private val numPartitions = 1


We can rename this test to be TopicCommandTest.

@ijuma , we already have a TopicCommandTest to do Unit test. This TopicCommandWithAdminClientTest is much like an integration test. How about rename to TopicCommandIntegrationTest just like DeleteOffsetsConsumerGroupCommandIntegrationTest in the same folder?

So, I rename the test to TopicCommandIntegrationTest, and move this test to integration test folder: core/src/test/scala/integration/kafka/admin. Thanks.

The name sounds good, but I think you don't have to move the test. We rely on tags instead of directory structure for distinguishing test types.

I see. Moved back now. Thanks.

…ration test

showuon · 2021-04-07T09:54:37Z

@ijuma , I've updated the PR to check --bootstrap-server option manually, so that the help text can print without arguments. Thanks.

showuon · 2021-04-16T13:34:04Z

@ijuma , could you please check this PR? Thank you.
Failed tests are all un-related.

    Build / JDK 15 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopicsWithManyPartitions()
    Build / JDK 8 and Scala 2.12 / kafka.admin.ReassignPartitionsIntegrationTest.testLogDirReassignment()

showuon · 2021-05-18T06:31:34Z

@ijuma , please help review this PR again. Thanks.

showuon · 2021-05-18T13:52:23Z

failed tests are unrelated and flaky. Thanks.

    Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.integration.BlockingConnectorTest.testBlockInSinkTaskStart
    Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.integration.BlockingConnectorTest.testBlockInConnectorStop
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.integration.SourceConnectorsIntegrationTest.testTopicsAreCreatedWhenAutoCreateTopicsIsEnabledAtTheBroker
    Build / JDK 11 and Scala 2.13 / kafka.integration.MetricsDuringTopicCreationDeletionTest.testMetricsDuringTopicCreateDelete()

ijuma · 2021-05-19T12:37:06Z

      if (!has(listOpt) && !has(describeOpt))
        CommandLineUtils.checkRequiredArgs(parser, options, topicOpt)
-      if (has(createOpt) && !has(replicaAssignmentOpt) && has(zkConnectOpt))
+      if (has(createOpt) && !has(replicaAssignmentOpt))


Was it a bug that we only verified this when zkConnectOpt was set? If so, we should add a unit test for this case.

Good catch! Added 2 tests: testCreateWithAssignmentAndPartitionCount and testCreateWithAssignmentAndReplicationFactor in TopicCommandTest. Thank you.

ijuma

Thanks for the updates. Just a few comments, I think we're close.

showuon · 2021-05-20T06:22:01Z

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

showuon · 2021-05-20T08:23:45Z

Failed tests are unrelated and flaky. Thanks.

    Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplicationWithEmptyPartition()
    Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic()
    Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testReplication()

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

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

ijuma

Thanks for the PR, LGTM.

ijuma · 2021-06-01T13:57:00Z

Unrelated failures:

Build / JDK 8 and Scala 2.12 / testReplicationWithEmptyPartition() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest
4s
Build / JDK 11 and Scala 2.13 / testCreateClusterAndCreateListDeleteTopic() – kafka.server.RaftClusterTest
15s
Build / JDK 15 and Scala 2.13 / testReplication() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest

ijuma · 2021-06-01T14:00:26Z

After merging, I noticed we are missing a note in upgrade.html. We can fix that as part of #10471.

ijuma · 2021-06-01T14:22:35Z

One more thing, did we check that there are no system tests using the flag we just removed?

showuon · 2021-06-02T10:03:22Z

Will address the above 2 comments in #10471. Thank you.

[KIP-464](https://cwiki.apache.org/confluence/display/KAFKA/KIP-464%3A+Defaults+for+AdminClient%23createTopic) (PR: #6728) made it possible to create topics without passing partition count and/or replica factor when using the admin client. We incorrectly disallowed this via #10457 while trying to ensure validation was consistent between ZK and the admin client (in this case the inconsistency was intentional). Fix this regression and add tests for the command lines in quick start (i.e. create topic and describe topic) to make sure it won't be broken in the future. Reviewers: Lee Dongjin <dongjin@apache.org>, Ismael Juma <ismael@juma.me.uk>

[KIP-464](https://cwiki.apache.org/confluence/display/KAFKA/KIP-464%3A+Defaults+for+AdminClient%23createTopic) (PR: apache/kafka#6728) made it possible to create topics without passing partition count and/or replica factor when using the admin client. We incorrectly disallowed this via apache/kafka#10457 while trying to ensure validation was consistent between ZK and the admin client (in this case the inconsistency was intentional). Fix this regression and add tests for the command lines in quick start (i.e. create topic and describe topic) to make sure it won't be broken in the future. Reviewers: Lee Dongjin <dongjin@apache.org>, Ismael Juma <ismael@juma.me.uk>

…he#11429) [KIP-464](https://cwiki.apache.org/confluence/display/KAFKA/KIP-464%3A+Defaults+for+AdminClient%23createTopic) (PR: apache#6728) made it possible to create topics without passing partition count and/or replica factor when using the admin client. We incorrectly disallowed this via apache#10457 while trying to ensure validation was consistent between ZK and the admin client (in this case the inconsistency was intentional). Fix this regression and add tests for the command lines in quick start (i.e. create topic and describe topic) to make sure it won't be broken in the future. Reviewers: Lee Dongjin <dongjin@apache.org>, Ismael Juma <ismael@juma.me.uk>

KAFKA-12588: remove --zookeeper option from topic command

47f0058

ijuma reviewed Apr 1, 2021

View reviewed changes

KAFKA-12596: remove unused trait and refactor codes

4213cfc

showuon commented Apr 2, 2021

View reviewed changes

ijuma reviewed Apr 2, 2021

View reviewed changes

KAFKA-12596: rename TopicCommandWithAdminClientTest and move to integ…

3b6cc27

…ration test

showuon force-pushed the KAFKA-12596 branch from cc8935f to 3b6cc27 Compare April 3, 2021 02:58

KAFKA-12596: move the integration test back to unit.kafka.admin

9488dff

showuon force-pushed the KAFKA-12596 branch from a010afb to 9488dff Compare April 3, 2021 03:31

showuon changed the title ~~KAFKA-12588: remove --zookeeper option from topic command~~ KAFKA-12596: remove --zookeeper option from topic command Apr 4, 2021

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

79712cb

showuon mentioned this pull request Apr 6, 2021

KAFKA-12597: remove deprecated zookeeper option in ReassignPartitionsCommand #10471

Merged

3 tasks

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

71a2648

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

a15b4a6

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

a70a06b

ijuma added the kraft label May 19, 2021

ijuma reviewed May 19, 2021

View reviewed changes

Comment thread core/src/test/scala/unit/kafka/admin/TopicCommandIntegrationTest.scala Outdated

ijuma reviewed May 19, 2021

View reviewed changes

KAFKA-12596: remove unused bootstrapOpt and add tests

f22e330

ijuma approved these changes Jun 1, 2021

View reviewed changes

ijuma merged commit 21532a7 into apache:trunk Jun 1, 2021

showuon deleted the KAFKA-12596 branch June 2, 2021 10:03

wenbingshen mentioned this pull request Jun 2, 2021

MINOR: update kafka-topics.sh line command tool upgrade notes with removed option #10806

Merged

3 tasks

rondagostino mentioned this pull request Jun 23, 2021

KAFKA-12756: Update ZooKeeper to v3.6.3 #10918

Merged

3 tasks

ladislavmacoun mentioned this pull request Feb 1, 2022

Unit tests fails with Kafka v3.0.0 confluentinc/librdkafka#3699

Closed

4 tasks

		@@ -65,9 +65,10 @@ class TopicCommandWithAdminClientTest extends KafkaServerTestHarness with Loggin
		private val numPartitions = 1

Conversation

showuon commented Apr 1, 2021

Committer Checklist (excluded from commit message)

Uh oh!

showuon commented Apr 1, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

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!

showuon commented May 18, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

showuon commented May 20, 2021

Uh oh!

showuon commented May 20, 2021

Uh oh!

showuon commented Jun 1, 2021

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jun 1, 2021

Uh oh!

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

Uh oh!

Uh oh!

ijuma commented Jun 1, 2021

Uh oh!

showuon commented Jun 2, 2021

Uh oh!

Reviewers

Assignees

showuon commented Apr 2, 2021 •

edited

Loading

ijuma commented Jun 1, 2021 •

edited

Loading