-
Notifications
You must be signed in to change notification settings - Fork 14.9k
KAFKA-19755 Move KRaftClusterTest from core module to server module [3/4] #21175
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
# Conflicts: # core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala # server/src/test/java/org/apache/kafka/server/KRaftClusterTest.java
|
@DL1231 Could you merge trunk in to trigger CI again? |
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
| values.get(resource).get(); | ||
| return ApiError.NONE; | ||
| } catch (ExecutionException e) { | ||
| return ApiError.fromThrowable(e.getCause()); |
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.
ApiError.fromThrowable already unwraps the exception, so we don't need to call getCause explictly
| } | ||
| } | ||
|
|
||
| private void assertListEquals(List<ApiError> expected, List<ApiError> actual) { |
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 we use assertIterableEquals(expected, actual); here instead?
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.
The reason I've opted for the custom assertListEquals method is that the incrementalAlterConfigs API does not guarantee the order of the ApiError objects in its returned list.
As you know, assertIterableEquals performs a strict comparison that requires the elements in both iterables to be in the exact same sequence.
The current assertListEquals implementation correctly verifies that the actual list contains all the expected errors and no unexpected ones, but it does so without being sensitive to their order.
|
|
||
| for (int i = 0; i < 3; i++) { | ||
| int brokerId = i; | ||
| TestUtils.waitForCondition( |
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.
Can it be replaced by cluster.waitForReadyBrokers();?
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.
Done.
Move KRaftClusterTest from core module to server module.
Rewrite
Reviewers: Chia-Ping Tsai chia7712@gmail.com