-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-18368 Remove TestUtils#MockZkConnect and remove zkConnect from TestUtils#createBrokerConfig #18352
base: trunk
Are you sure you want to change the base?
Conversation
This PR should wait for KAFKA-18730 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m1a2st thanks for this patch!
@@ -179,7 +183,26 @@ class CustomQuotaCallbackTest extends IntegrationTestHarness with SaslSetup { | |||
|
|||
private def createTopic(topic: String, numPartitions: Int, leader: Int): Unit = { | |||
val assignment = (0 until numPartitions).map { i => i -> Seq(leader) }.toMap | |||
TestUtils.createTopic(null, topic, assignment, servers) | |||
val adminZkClient = new AdminZkClient(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test is disabled, so we can just comment the code TestUtils.createTopic(null, topic, assignment, servers)
@@ -173,14 +173,14 @@ class KafkaApisTest extends Logging { | |||
overrideProperties: Map[String, String] = Map.empty, | |||
featureVersions: Seq[FeatureVersion] = Seq.empty): KafkaApis = { | |||
val properties = if (raftSupport) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please remove raftSupport
as well?
# Conflicts: # core/src/test/scala/unit/kafka/server/DynamicBrokerConfigTest.scala
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m1a2st thanks for update. This PR removes several test cases. We should carefully review each case to determine if it's exclusively related to ZooKeeper or if it should be rewritten with KRaft. If a more thorough investigation is required, we can create a follow-up issue to address the removal of the raftSupport flag.
@@ -95,9 +95,40 @@ class DynamicBrokerConfigTest { | |||
} | |||
} | |||
|
|||
@Test | |||
def testEnableDefaultUncleanLeaderElection(): Unit = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we did not have such test case before?
@@ -614,136 +288,12 @@ class KafkaApisTest extends Logging { | |||
assertEquals(cgConfigs.size, configs.size) | |||
} | |||
|
|||
@Test | |||
def testAlterConfigsClientMetrics(): Unit = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why to remove this test case? Does this scenario belong to zk only?
Thanks for @chia7712 comment, I open a Jira to trace it. |
as title
Committer Checklist (excluded from commit message)