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 2020/04/22 03:38:37 UTC
[GitHub] [kafka] d8tltanc opened a new pull request #8527: Splitted unit tests for --zookeeper flag removal
d8tltanc opened a new pull request #8527:
URL: https://github.com/apache/kafka/pull/8527
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### 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] cmccabe commented on pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#issuecomment-623716775
----------------------------------------------------------------
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] cmccabe commented on a change in pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#discussion_r413718893
##########
File path: core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
##########
@@ -227,8 +227,10 @@ class DeleteTopicTest extends ZooKeeperTestHarness {
}, s"Not all replicas for topic $topic are in states of either ReplicaDeletionSuccessful or OfflineReplica")
// increase the partition count for topic
- val topicCommandOptions = new TopicCommand.TopicCommandOptions(Array("--zookeeper", zkConnect, "--alter", "--topic", topic, "--partitions", "2"))
- new ZookeeperTopicService(zkClient).alterTopic(topicCommandOptions)
+ 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)
Review comment:
There should be a "get" here to ensure that we actually create the partitions before shutting down, right?
----------------------------------------------------------------
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] cmccabe commented on pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#issuecomment-622610148
ok to test
----------------------------------------------------------------
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] d8tltanc commented on pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
d8tltanc commented on pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#issuecomment-622330041
ok to test
----------------------------------------------------------------
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] cmccabe commented on a change in pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#discussion_r413720950
##########
File path: core/src/test/scala/unit/kafka/admin/TopicCommandWithAdminClientTest.scala
##########
@@ -293,13 +293,6 @@ class TopicCommandWithAdminClientTest extends KafkaServerTestHarness with Loggin
}
}
- @Test
- def testCreateWithUnspecifiedReplicationFactorAndPartitionsWithZkClient(): Unit = {
Review comment:
It's weird that this is even in this file. This should be moved to `TopicCommandWithZKClientTest.scala`, I think
----------------------------------------------------------------
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] cmccabe commented on pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#issuecomment-622266298
ok to test
----------------------------------------------------------------
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] d8tltanc commented on pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
d8tltanc commented on pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#issuecomment-619308386
ok to test
----------------------------------------------------------------
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] cmccabe commented on issue #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on issue #8527:
URL: https://github.com/apache/kafka/pull/8527#issuecomment-618334410
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.
----------------------------------------------------------------
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] d8tltanc commented on a change in pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
d8tltanc commented on a change in pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#discussion_r414945297
##########
File path: core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
##########
@@ -227,8 +227,10 @@ class DeleteTopicTest extends ZooKeeperTestHarness {
}, s"Not all replicas for topic $topic are in states of either ReplicaDeletionSuccessful or OfflineReplica")
// increase the partition count for topic
- val topicCommandOptions = new TopicCommand.TopicCommandOptions(Array("--zookeeper", zkConnect, "--alter", "--topic", topic, "--partitions", "2"))
- new ZookeeperTopicService(zkClient).alterTopic(topicCommandOptions)
+ 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)
Review comment:
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.
----------------------------------------------------------------
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] cmccabe commented on a change in pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#discussion_r413719892
##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsCommandArgsTest.scala
##########
@@ -255,7 +225,7 @@ class ReassignPartitionsCommandArgsTest {
@Test
def shouldNotAllowBrokersListWithVerifyOption(): Unit = {
val args = Array(
- "--zookeeper", "localhost:1234",
+ "--bootstrap-server", "localhost:1234",
Review comment:
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
##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsCommandArgsTest.scala
##########
@@ -275,7 +245,7 @@ class ReassignPartitionsCommandArgsTest {
@Test
def shouldNotAllowTopicsOptionWithVerify(): Unit = {
val args = Array(
- "--zookeeper", "localhost:1234",
+ "--bootstrap-server", "localhost:1234",
Review comment:
keep this change
##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsCommandArgsTest.scala
##########
@@ -265,7 +235,7 @@ class ReassignPartitionsCommandArgsTest {
@Test
def shouldNotAllowThrottleWithVerifyOption(): Unit = {
val args = Array(
- "--zookeeper", "localhost:1234",
+ "--bootstrap-server", "localhost:1234",
Review comment:
keep this change
----------------------------------------------------------------
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] cmccabe commented on pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#issuecomment-620310659
thanks for revising this. Let's get rid of the `@Ignore` on the tests here since we still do have the --zookeeper flags
----------------------------------------------------------------
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] cmccabe commented on pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#issuecomment-619322065
ok to test
----------------------------------------------------------------
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] d8tltanc commented on a change in pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
d8tltanc commented on a change in pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#discussion_r414844095
##########
File path: core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala
##########
@@ -1408,13 +1408,23 @@ class DynamicBrokerReconfigurationTest extends ZooKeeperTestHarness with SaslSet
sslStoreProps.put(KafkaConfig.PasswordEncoderSecretProp, kafkaConfig.passwordEncoderSecret.map(_.value).orNull)
zkClient.makeSurePersistentPathExists(ConfigEntityChangeNotificationZNode.path)
- val args = Array("--zookeeper", kafkaConfig.zkConnect,
- "--alter", "--add-config", sslStoreProps.asScala.map { case (k, v) => s"$k=$v" }.mkString(","),
- "--entity-type", "brokers",
- "--entity-name", kafkaConfig.brokerId.toString)
- ConfigCommand.main(args)
+ val entityType = ConfigType.Broker
+ val entityName = kafkaConfig.brokerId.toString
+ val passwordConfigs = sslStoreProps.asScala.keySet.filter(DynamicBrokerConfig.isPasswordConfig)
+ val passwordEncoderConfigs = new Properties
+ passwordEncoderConfigs ++= sslStoreProps.asScala.filter { case (key, _) => key.startsWith("password.encoder.") }
Review comment:
Good catch. Deleted.
----------------------------------------------------------------
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] d8tltanc commented on a change in pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
d8tltanc commented on a change in pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#discussion_r414952990
##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsCommandArgsTest.scala
##########
@@ -255,7 +225,7 @@ class ReassignPartitionsCommandArgsTest {
@Test
def shouldNotAllowBrokersListWithVerifyOption(): Unit = {
val args = Array(
- "--zookeeper", "localhost:1234",
+ "--bootstrap-server", "localhost:1234",
Review comment:
ok
##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsCommandArgsTest.scala
##########
@@ -265,7 +235,7 @@ class ReassignPartitionsCommandArgsTest {
@Test
def shouldNotAllowThrottleWithVerifyOption(): Unit = {
val args = Array(
- "--zookeeper", "localhost:1234",
+ "--bootstrap-server", "localhost:1234",
Review comment:
ok
##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsCommandArgsTest.scala
##########
@@ -275,7 +245,7 @@ class ReassignPartitionsCommandArgsTest {
@Test
def shouldNotAllowTopicsOptionWithVerify(): Unit = {
val args = Array(
- "--zookeeper", "localhost:1234",
+ "--bootstrap-server", "localhost:1234",
Review comment:
ok
----------------------------------------------------------------
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] cmccabe commented on issue #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on issue #8527:
URL: https://github.com/apache/kafka/pull/8527#issuecomment-618337731
ok to test
----------------------------------------------------------------
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] d8tltanc commented on a change in pull request #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
d8tltanc commented on a change in pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#discussion_r414955081
##########
File path: core/src/test/scala/unit/kafka/admin/TopicCommandWithAdminClientTest.scala
##########
@@ -293,13 +293,6 @@ class TopicCommandWithAdminClientTest extends KafkaServerTestHarness with Loggin
}
}
- @Test
- def testCreateWithUnspecifiedReplicationFactorAndPartitionsWithZkClient(): Unit = {
Review comment:
Yes. If we are ignoring the tests instead of deleting the tests, we should move this one into TopicCommandWithZKClientTest.scala
----------------------------------------------------------------
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] cmccabe commented on issue #8527: Remove dependencies on deprecated --zookeeper command flags in junit tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on issue #8527:
URL: https://github.com/apache/kafka/pull/8527#issuecomment-618338178
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.
----------------------------------------------------------------
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] cmccabe commented on a change in pull request #8527: Splitted unit tests for --zookeeper flag removal
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #8527:
URL: https://github.com/apache/kafka/pull/8527#discussion_r413713907
##########
File path: core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala
##########
@@ -1408,13 +1408,23 @@ class DynamicBrokerReconfigurationTest extends ZooKeeperTestHarness with SaslSet
sslStoreProps.put(KafkaConfig.PasswordEncoderSecretProp, kafkaConfig.passwordEncoderSecret.map(_.value).orNull)
zkClient.makeSurePersistentPathExists(ConfigEntityChangeNotificationZNode.path)
- val args = Array("--zookeeper", kafkaConfig.zkConnect,
- "--alter", "--add-config", sslStoreProps.asScala.map { case (k, v) => s"$k=$v" }.mkString(","),
- "--entity-type", "brokers",
- "--entity-name", kafkaConfig.brokerId.toString)
- ConfigCommand.main(args)
+ val entityType = ConfigType.Broker
+ val entityName = kafkaConfig.brokerId.toString
+ val passwordConfigs = sslStoreProps.asScala.keySet.filter(DynamicBrokerConfig.isPasswordConfig)
+ val passwordEncoderConfigs = new Properties
+ passwordEncoderConfigs ++= sslStoreProps.asScala.filter { case (key, _) => key.startsWith("password.encoder.") }
Review comment:
Hmm. Do we need this if we have `passwordConfigs`?
----------------------------------------------------------------
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