MINOR: add helper function for clusterInstance #16852
Conversation
| @@ -187,6 +188,21 @@ default void waitTopicDeletion(String topic) throws InterruptedException { | |||
| waitForTopic(topic, 0); | |||
| } | |||
|
|
|||
| default long waitForMeatdataSync() throws InterruptedException { | |||
There was a problem hiding this comment.
It is totally unused. Why we need this helper?
There was a problem hiding this comment.
Yes, it is not used now.
This comes from TestUtils.ensureConsistentKRaftMetadata and I think it can help new infra user easy to sync metadata.
WDYT?
|
@TaiJuWu any updates? |
| @@ -187,6 +188,14 @@ default void waitTopicDeletion(String topic) throws InterruptedException { | |||
| waitForTopic(topic, 0); | |||
| } | |||
|
|
|||
| long waitForMeatdataSync() throws InterruptedException; | |||
There was a problem hiding this comment.
waitForMetadataSync
Also, what is the purpose of the return value?
| clusterInstance.createTopic(topicName, partitions, replicas); | ||
| clusterInstance.waitForTopic(topicName, partitions); | ||
|
|
||
| long offset = clusterInstance.waitForMeatdataSync(); |
There was a problem hiding this comment.
Could you please write test to make sure "it works well" rather than checking the return value. For example, the method is used to "wait" the metadata. Hence, we should make some metadata changes and then call waitForMetadataSync. Finally, we call the APIs using the metadata to check the metadata is synced.
| @@ -187,6 +188,14 @@ default void waitTopicDeletion(String topic) throws InterruptedException { | |||
| waitForTopic(topic, 0); | |||
| } | |||
|
|
|||
| void waitForMeatdataSync(String topic, int partition, int isrSize) throws InterruptedException; | |||
There was a problem hiding this comment.
this is almost equal to waitForTopic, right?
There was a problem hiding this comment.
Yes, I find KRaft can trace cluster log by controller but there is no way to check cluster log for ZK.
ZK only support partition level log so I am so hesitate to add this method.
We can discuss offline tonight!
There was a problem hiding this comment.
this is almost equal to
waitForTopic, right?
Hi @chia7712 , I removed waitForMetadataSync, PTAL.
|
|
||
| default void createTopic(String topicName, int partitions, short replicas) { | ||
| try (Admin admin = createAdminClient()) { | ||
| admin.createTopics(Collections.singletonList(new NewTopic(topicName, partitions, replicas))); |
There was a problem hiding this comment.
why not calling the wait?
There was a problem hiding this comment.
There is no specific reason.
Personally, I prefer user to waitTopic explicitly.
I am also fine to sync topic implicitly, just rename to CreateTopicWithSync.
WDYT?
There was a problem hiding this comment.
This is a "helper" method, so I feel a little sugar is good to users. We can add comments to this helper to remind users that this method is blocked until metadata get synced.
There was a problem hiding this comment.
Thanks for your explanation and review, updated.
| clusterInstance.createTopic(topicName, partitions, replicas); | ||
|
|
||
| try (Admin admin = clusterInstance.createAdminClient()) { | ||
| Assertions.assertTrue(admin.listTopics().listings().get().stream().anyMatch(s -> s.name().equals(topicName))); |
There was a problem hiding this comment.
Please check the partition and replica also
There was a problem hiding this comment.
Updated. Thanks for review.
…6852) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
*More detailed description of your change,
Add two helper function:
clusterInstance#createTopicSummary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)