mimaison commented on code in PR #18166:
URL: https://github.com/apache/kafka/pull/18166#discussion_r1886634250
##########
core/src/test/scala/integration/kafka/api/CustomQuotaCallbackTest.scala:
##########
@@ -42,6 +41,7 @@ import java.{lang, util}
import scala.collection.mutable.ArrayBuffer
import scala.jdk.CollectionConverters._
+@Disabled
Review Comment:
-> `@Disabled("KAFKA-18213")`
##########
core/src/test/scala/unit/kafka/server/BaseRequestTest.scala:
##########
@@ -72,13 +72,7 @@ abstract class BaseRequestTest extends
IntegrationTestHarness {
* For KRaft clusters that is any broker as the broker will forward the
request to the active
* controller. For Legacy clusters that is the controller broker.
*/
- def adminSocketServer: SocketServer = {
- if (isKRaftTest()) {
- anySocketServer
- } else {
- controllerSocketServer
- }
- }
+ def adminSocketServer: SocketServer = anySocketServer
Review Comment:
Can we adjust the comment?
##########
core/src/test/scala/integration/kafka/server/QuorumTestHarness.scala:
##########
@@ -482,18 +365,6 @@ abstract class QuorumTestHarness extends Logging {
Configuration.setConfiguration(null)
faultHandler.maybeRethrowFirstException()
}
-
- // Trigger session expiry by reusing the session id in another client
- def createZooKeeperClientToTriggerSessionExpiry(zooKeeper: ZooKeeper):
ZooKeeper = {
- val dummyWatcher = new Watcher {
- override def process(event: WatchedEvent): Unit = {}
- }
- val anotherZkClient = new ZooKeeper(zkConnect, 1000, dummyWatcher,
- zooKeeper.getSessionId,
- zooKeeper.getSessionPasswd)
- assertNull(anotherZkClient.exists("/nonexistent", false)) // Make sure new
client works
- anotherZkClient
- }
}
object QuorumTestHarness {
Review Comment:
Can we also remove the `ZkClientEventThreadSuffix` field from this object?
Or is it still used?
##########
core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala:
##########
@@ -176,30 +165,18 @@ abstract class KafkaServerTestHarness extends
QuorumTestHarness {
topicConfig: Properties = new Properties,
listenerName: ListenerName = listenerName,
adminClientConfig: Properties = new Properties
- ): scala.collection.immutable.Map[Int, Int] = {
- if (isKRaftTest()) {
- Using.resource(createAdminClient(brokers, listenerName,
adminClientConfig)) { admin =>
- TestUtils.createTopicWithAdmin(
- admin = admin,
- topic = topic,
- brokers = brokers,
- controllers = controllerServers,
- numPartitions = numPartitions,
- replicationFactor = replicationFactor,
- topicConfig = topicConfig
- )
- }
- } else {
- TestUtils.createTopic(
- zkClient = zkClient,
+ ): scala.collection.immutable.Map[Int, Int] =
Review Comment:
Let's keep the brackets around the method here too.
##########
core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala:
##########
@@ -102,20 +98,17 @@ abstract class IntegrationTestHarness extends
KafkaServerTestHarness {
}
}
- private def insertControllerListenersIfNeeded(props: Seq[Properties]): Unit
= {
- if (isKRaftTest()) {
- props.foreach { config =>
- // Add a security protocol for the controller endpoints, if one is not
already set.
- val securityPairs =
config.getProperty(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG,
"").split(",")
- val toAdd =
config.getProperty(KRaftConfigs.CONTROLLER_LISTENER_NAMES_CONFIG,
"").split(",").filter(
- e => !securityPairs.exists(_.startsWith(s"$e:")))
- if (toAdd.nonEmpty) {
-
config.setProperty(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG,
(securityPairs ++
- toAdd.map(e =>
s"$e:${controllerListenerSecurityProtocol.toString}")).mkString(","))
- }
+ private def insertControllerListenersIfNeeded(props: Seq[Properties]): Unit
=
Review Comment:
Can we keep the curly brackets `{` and `}` around the method?
```
private def insertControllerListenersIfNeeded(props: Seq[Properties]): Unit
= {
...
}
```
##########
core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala:
##########
@@ -176,30 +165,18 @@ abstract class KafkaServerTestHarness extends
QuorumTestHarness {
topicConfig: Properties = new Properties,
listenerName: ListenerName = listenerName,
adminClientConfig: Properties = new Properties
- ): scala.collection.immutable.Map[Int, Int] = {
- if (isKRaftTest()) {
- Using.resource(createAdminClient(brokers, listenerName,
adminClientConfig)) { admin =>
- TestUtils.createTopicWithAdmin(
- admin = admin,
- topic = topic,
- brokers = brokers,
- controllers = controllerServers,
- numPartitions = numPartitions,
- replicationFactor = replicationFactor,
- topicConfig = topicConfig
- )
- }
- } else {
- TestUtils.createTopic(
- zkClient = zkClient,
+ ): scala.collection.immutable.Map[Int, Int] =
Review Comment:
Same for the other methods below
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
