Skip to content

KAFKA-18226: Disable CustomQuotaCallbackTest and remove isKRaftTest#18166

Merged
mimaison merged 22 commits into
apache:trunkfrom
m1a2st:KAFKA-18226
Dec 16, 2024
Merged

KAFKA-18226: Disable CustomQuotaCallbackTest and remove isKRaftTest#18166
mimaison merged 22 commits into
apache:trunkfrom
m1a2st:KAFKA-18226

Conversation

@m1a2st

@m1a2st m1a2st commented Dec 13, 2024

Copy link
Copy Markdown
Collaborator

Jira: https://issues.apache.org/jira/browse/KAFKA-18226

We change all tests to kraft mode, thus we didn't need to check which test is zk, we can remvoe check zk condition

Committer Checklist (excluded from commit message)

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

@github-actions github-actions Bot added triage PRs from the community core Kafka Broker tests Test fixes (including flaky tests) labels Dec 13, 2024
TestUtils.createBrokerConfigs(
numServers,
zkConnectOrNull,
null,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we even remove the argument from createBrokerConfigs()?

@m1a2st m1a2st Dec 15, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The createBrokerConfigs() will modify about 40 files, I will address it on another PR and open another Jira to trace it.

Update: I open Jira: KAFKA-18261

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests are still using not null value on this method.

val config = KafkaConfig.fromProps(TestUtils.createBrokerConfig(brokerId, "localhost:1234"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, then we can do this in another PR. Thanks

@mimaison mimaison left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I left a few comments.

controllerSocketServer
}
}
def adminSocketServer: SocketServer = anySocketServer

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we adjust the comment?

import scala.collection.mutable.ArrayBuffer
import scala.jdk.CollectionConverters._

@Disabled

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> @Disabled("KAFKA-18213")

config.setProperty(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG, (securityPairs ++
toAdd.map(e => s"$e:${controllerListenerSecurityProtocol.toString}")).mkString(","))
}
private def insertControllerListenersIfNeeded(props: Seq[Properties]): Unit =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep the curly brackets { and } around the method?

private def insertControllerListenersIfNeeded(props: Seq[Properties]): Unit = {
...
}

}
}

object QuorumTestHarness {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also remove the ZkClientEventThreadSuffix field from this object? Or is it still used?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still use on this, but I think we can remove it

QuorumTestHarness.ZkClientEventThreadSuffix,

} else {
TestUtils.createTopic(
zkClient = zkClient,
): scala.collection.immutable.Map[Int, Int] =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the brackets around the method here too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for the other methods below

@mimaison mimaison left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, LGTM

@mimaison mimaison merged commit 92f61b3 into apache:trunk Dec 16, 2024
mimaison pushed a commit that referenced this pull request Dec 16, 2024
…18166)


Reviewers: Mickael Maison <mickael.maison@gmail.com>
@chia7712 chia7712 mentioned this pull request Dec 18, 2024
3 tasks
@github-actions github-actions Bot removed the triage PRs from the community label Dec 19, 2024
tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-approved core Kafka Broker tests Test fixes (including flaky tests)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants