-
Notifications
You must be signed in to change notification settings - Fork 14.9k
KAFKA-19755 Move KRaftClusterTest from core module to server module [4/4] #21230
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
Conversation
chia7712
left a comment
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.
@DL1231 thanks for this patch!
server/src/test/java/org/apache/kafka/server/KRaftClusterTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/apache/kafka/server/KRaftClusterTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/apache/kafka/server/KRaftClusterTest.java
Outdated
Show resolved
Hide resolved
| var controller = cluster.controllers().values().iterator().next(); | ||
| controller.controller().waitForReadyBrokers(3).get(); | ||
| TestUtils.retryOnExceptionWithTimeout(60000, () -> { | ||
| var latch = QuorumControllerIntegrationTestUtils.pause((QuorumController) controller.controller()); |
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.
I'm not a fan of adding an entire dependency for such a minor reference. Maybe we can duplicate a bit of code as a trade-off?
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.
That's a fair point. I actually considered duplicating the code as well, but it leads to two issues:
- We would need to change the visibility of
QuorumController#appendControlEventto public. - We would still need to reference
QuorumController, which means the dependency is unavoidable.
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.
Removing the usage of QuorumControllerIntegrationTestUtils decouples the dependency on the metadata module's test scope
Move KRaftClusterTest from core module to server module.
Rewrite
Reviewers: Chia-Ping Tsai [email protected]