Remove dependencies on deprecated --zookeeper command flags in junit tests by ctan888 · Pull Request #8527 · apache/kafka

ctan888 · 2020-04-22T03:38:27Z

Removed the --zookeeper flag related tests. Scram credential related tests are also get removed since they are using --zookeeper flag.

Committer Checklist (excluded from commit message)

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

cmccabe · 2020-04-23T10:50:35Z


+    val passwordConfigs = sslStoreProps.asScala.keySet.filter(DynamicBrokerConfig.isPasswordConfig)
+    val passwordEncoderConfigs = new Properties
+    passwordEncoderConfigs ++= sslStoreProps.asScala.filter { case (key, _) => key.startsWith("password.encoder.") }


Hmm. Do we need this if we have passwordConfigs?

Good catch. Deleted.

cmccabe · 2020-04-23T10:58:02Z

For ConfigCommandTest.scala, we should keep the tests of the --zookeeper command since that is still present in trunk.

The goal should be to remove unnecessary dependencies on the deprecated zookeeper command code.

cmccabe · 2020-04-23T10:58:43Z

+    val props = new Properties()
+    props.setProperty(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG, TestUtils.getBrokerListStrFromServers(servers))
+    val adminClient = Admin.create(props)
+    adminClient.createPartitions(Map(topic -> NewPartitions.increaseTo(2)).asJava)


There should be a "get" here to ensure that we actually create the partitions before shutting down, right?

Good catch. Although this will return an API exception (can't increase partition number during topic deletion), we still need to wait for the completion.

cmccabe · 2020-04-23T11:00:22Z

  def shouldNotAllowBrokersListWithVerifyOption(): Unit = {
    val args = Array(
-      "--zookeeper", "localhost:1234",
+      "--bootstrap-server", "localhost:1234",


This is a good change since the test is not specifically testing the legacy mode (so it should use the new mode) Keep this one

cmccabe · 2020-04-23T11:00:30Z

  def shouldNotAllowTopicsOptionWithVerify(): Unit = {
    val args = Array(
-      "--zookeeper", "localhost:1234",
+      "--bootstrap-server", "localhost:1234",


keep this change

cmccabe · 2020-04-23T11:00:37Z

  def shouldNotAllowThrottleWithVerifyOption(): Unit = {
    val args = Array(
-      "--zookeeper", "localhost:1234",
+      "--bootstrap-server", "localhost:1234",


keep this change

cmccabe · 2020-04-23T11:02:00Z

  }

-  @Test
-  def testCreateWithUnspecifiedReplicationFactorAndPartitionsWithZkClient(): Unit = {


It's weird that this is even in this file. This should be moved to TopicCommandWithZKClientTest.scala, I think

Yes. If we are ignoring the tests instead of deleting the tests, we should move this one into TopicCommandWithZKClientTest.scala

cmccabe · 2020-04-23T11:05:17Z

ok to test

cmccabe · 2020-04-23T11:06:18Z

Looks like the JDK8 build flaked.

11:00:07 Error occurred during initialization of VM
11:00:07 java.lang.OutOfMemoryError: unable to create new native thread

We've got some things to fix before re-running in any case.

ctan888 · 2020-04-25T02:43:44Z

ok to test

cmccabe · 2020-04-25T05:06:12Z

ok to test

cmccabe · 2020-04-28T00:45:30Z

thanks for revising this. Let's get rid of the @Ignore on the tests here since we still do have the --zookeeper flags

cmccabe · 2020-05-01T06:18:59Z

ok to test

ctan888 · 2020-05-01T10:12:32Z

ok to test

cmccabe · 2020-05-01T23:29:29Z

ok to test

cmccabe · 2020-05-04T21:28:41Z

LGTM. Thanks, @d8tltanc.

cmccabe · 2020-05-04T21:28:51Z

JDK14 failure seems like infra issue

@test

* 'trunk' of github.com:apache/kafka: (87 commits) KAFKA-9865: Expose output topic names from TopologyTestDriver (apache#8483) MINOR - Increase the number of Trogdor Histogram buckets to 10000 (apache#8627) KAFKA-9768: Fix handling of rest.advertised.listener config (apache#8360) KAFKA-9419: Fix possible integer overflow in CircularIterator (apache#7950) MINOR: Only add 'Data' suffix for generated request/response/header types (apache#8625) KAFKA-9947; Ensure proper shutdown of services in `TransactionsBounceTest` (apache#8602) KAFKA-6342; Remove unused workaround for JSON parsing of non-escaped strings (apache#8591) MINOR: Pass `-release 8` to scalac and upgrade to Gradle 6.4 (apache#8538) KAFKA-9946; Partition deletion event should only be sent if deletion was requested in the StopReplica request (apache#8609) MINOR: Improve TopologyTestDriver JavaDocs (apache#8619) MINOR: MockAdminClient should return InvalidReplicationFactorException if brokers.size < replicationFactor KAFKA-9748: Add Streams eos-beta integration test (apache#8496) KAFKA-9731: Disable immediate fetch response for hw propagation if replica selector is not defined (apache#8607) HOTFIX: set correct numIterations in shouldAllowConcurrentAccesses MINOR: Clean up some test dependencies on ConfigCommand and TopicCommand (apache#8527) KAFKA-9918; SslEngineFactory is NOT closed when channel is closing (apache#8551) KAFKA-9798: Send one round synchronously before starting the async producer (apache#8565) MINOR: Annotate KafkaAdminClientTest.testAlterClientQuotas() with @test KAFKA-9589: Enable testLogAppendTimeNonCompressedV2 and fix bug in helper method (apache#8533) MINOR: Use min/max function when possible (apache#8577) ... # Conflicts: # core/src/main/scala/kafka/log/Log.scala # gradle/dependencies.gradle # gradle/wrapper/gradle-wrapper.properties # gradlew

…and (apache#8527) Avoid calling into ConfigCommand and TopicCommand from tests that are not related to these commands. It's better to just invoke the admin APIs. Change a few cases where we were testing the deprecated --zookeeper flag to testing the --bootstrap-server flag instead. Unless we're explicitly testing the deprecated code path, we should be using the non-deprecated flags. Move testCreateWithUnspecifiedReplicationFactorAndPartitionsWithZkClient from TopicCommandWithAdminClientTest.scala into TopicCommandWithZKClientTest.scala, since it makes more sense in the latter. Reviewers: Colin P. McCabe <cmccabe@apache.org>

Splitted unit tests for --zookeeper flag removal

9385a65

cmccabe reviewed Apr 23, 2020

View reviewed changes

cmccabe changed the title ~~Splitted unit tests for --zookeeper flag removal~~ Remove dependencies on deprecated --zookeeper command flags in junit tests Apr 23, 2020

cmccabe reviewed Apr 23, 2020

View reviewed changes

ctan888 added 2 commits April 24, 2020 13:07

Merge branch 'trunk' into HEAD

ccb7bb2

It gnore the tests instead of deletion

f8f7bdb

ctan888 force-pushed the REMOVE_UNIT branch from d8e016d to f8f7bdb Compare April 25, 2020 02:41

ctan888 force-pushed the REMOVE_UNIT branch 3 times, most recently from 2c59469 to c4ffd05 Compare April 30, 2020 20:55

Remove decorator

920eec4

ctan888 force-pushed the REMOVE_UNIT branch from c4ffd05 to 920eec4 Compare April 30, 2020 20:57

cmccabe mentioned this pull request May 1, 2020

Remove unit #8601

Closed

cmccabe merged commit 7cb1600 into apache:trunk May 4, 2020

Uh oh!

Conversation

ctan888 commented Apr 22, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Apr 23, 2020

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!

cmccabe commented Apr 23, 2020

Uh oh!

cmccabe commented Apr 23, 2020

Uh oh!

ctan888 commented Apr 25, 2020

Uh oh!

cmccabe commented Apr 25, 2020

Uh oh!

cmccabe commented Apr 28, 2020

Uh oh!

cmccabe commented May 1, 2020

Uh oh!

ctan888 commented May 1, 2020

Uh oh!

cmccabe commented May 1, 2020

Uh oh!

cmccabe commented May 4, 2020

Uh oh!

cmccabe commented May 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ctan888 commented Apr 22, 2020 •

edited

Loading