-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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-16550: add integration test for LogDirsCommand #16485
Conversation
tools/src/test/java/org/apache/kafka/tools/LogDirsCommandTest.java
Outdated
Show resolved
Hide resolved
tools/src/test/java/org/apache/kafka/tools/LogDirsCommandTest.java
Outdated
Show resolved
Hide resolved
49b5018
to
4af149a
Compare
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.
@FrankYang0529 thanks for this patch
} | ||
|
||
@ClusterTest | ||
public void testLogDirsWithSpecificTopic(ClusterInstance clusterInstance) { |
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 add test to verify that it returns only the specific topic will get returned? for example, you can create another topic before testing.
// check all log dirs and topic partitions are present | ||
Map<Integer, Map<String, LogDirDescription>> logDirs = assertDoesNotThrow(() -> admin.describeLogDirs(clusterInstance.brokerIds()).allDescriptions().get()); | ||
assertFalse(logDirs.isEmpty()); | ||
logDirs.forEach((brokerId, logDirInfo) -> { |
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.
please remove {}
// check log dir and topic partition are present | ||
Map<Integer, Map<String, LogDirDescription>> logDirs = assertDoesNotThrow(() -> admin.describeLogDirs(Collections.singleton(brokerId)).allDescriptions().get()); | ||
assertEquals(1, logDirs.size()); | ||
logDirs.forEach((brokerIdValue, logDirInfo) -> { |
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.
please remove {}
// check log dir is present, but topic partition is not | ||
Map<Integer, Map<String, LogDirDescription>> logDirs = assertDoesNotThrow(() -> admin.describeLogDirs(clusterInstance.brokerIds()).allDescriptions().get()); | ||
assertFalse(logDirs.isEmpty()); | ||
logDirs.forEach((brokerId, logDirInfo) -> { |
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.
please remove {}
Signed-off-by: PoAn Yang <[email protected]>
4af149a
to
a22e217
Compare
Hi @chia7712, I address all comments. Could you take a look again? Thank you. |
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.
LGTM
Reviewers: Chia-Ping Tsai <[email protected]>
Reviewers: Chia-Ping Tsai <[email protected]>
Add integration test cases for LogDirsCommand:
Committer Checklist (excluded from commit message)