-
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
MINOR: rewrite TopicBasedRemoteLogMetadataManagerTest by ClusterTestE… #15917
Conversation
...a/org/apache/kafka/server/log/remote/metadata/storage/RemoteLogMetadataManagerTestUtils.java
Outdated
Show resolved
Hide resolved
...a/org/apache/kafka/server/log/remote/metadata/storage/RemoteLogMetadataManagerTestUtils.java
Outdated
Show resolved
Hide resolved
...a/org/apache/kafka/server/log/remote/metadata/storage/RemoteLogMetadataManagerTestUtils.java
Show resolved
Hide resolved
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.
Left minor comments. PTAL. Thanks!
.../apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerTest.java
Show resolved
Hide resolved
...a/org/apache/kafka/server/log/remote/metadata/storage/RemoteLogMetadataManagerTestUtils.java
Show resolved
Hide resolved
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.
Thanks for the improvement. Left some comments.
.../apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerTest.java
Outdated
Show resolved
Hide resolved
.../apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerTest.java
Outdated
Show resolved
Hide resolved
.../apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerTest.java
Outdated
Show resolved
Hide resolved
...ache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerHarness.java
Outdated
Show resolved
Hide resolved
...a/org/apache/kafka/server/log/remote/metadata/storage/RemoteLogMetadataManagerTestUtils.java
Outdated
Show resolved
Hide resolved
|
||
void waitForReadyBrokers() throws InterruptedException; | ||
|
||
default void waitForTopic(String topic, int partitions) throws InterruptedException { |
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.
@showuon I add a helper to make all ClusterInstance
users :)
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.
Thanks for the update.
String logDir = TestUtils.tempDirectory("rlmm_segs_").getAbsolutePath(); | ||
TopicBasedRemoteLogMetadataManager topicBasedRemoteLogMetadataManager = new TopicBasedRemoteLogMetadataManager(startConsumerThread); | ||
|
||
System.out.println("[CHIA] " + bootstrapServers); |
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.
debug log
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.
(facepalm)
topicBasedRemoteLogMetadataManager = RemoteLogMetadataManagerTestUtils.builder() | ||
.topicIdPartitions(topicIdPartitions) | ||
.bootstrapServers(bootstrapServers(listenerName())) | ||
.startConsumerThread(startConsumerThread) | ||
.remoteLogMetadataTopicPartitioner(remoteLogMetadataTopicPartitioner) | ||
.build(); |
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.
Thanks for the refactor!
@showuon Please take a look if you have free time. I'd like to migrate all tests of storage to new test infra after this PR gets merged. |
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!
I will merge #15962 first and then rebase this PR |
…xtensions (apache#15917) Reviewers: Kamal Chandraprakash <kamal.chandraprakash@gmail.com>, Luke Chen <showuon@gmail.com>
…xtensions (apache#15917) Reviewers: Kamal Chandraprakash <kamal.chandraprakash@gmail.com>, Luke Chen <showuon@gmail.com>
…xtensions (apache#15917) Reviewers: Kamal Chandraprakash <kamal.chandraprakash@gmail.com>, Luke Chen <showuon@gmail.com>
This is the first PR used to migrate tests of storage to new test infra. With the new test infra, we can make those tests run on either zk or kraft cluster easily.
Committer Checklist (excluded from commit message)