Repository navigation
Conversation
ClusterInstance#createTopic and #createTopicWithAssignment did not wait for the CreateTopics result, so a failed creation was hidden behind the 60 second waitTopicCreation timeout. Call get() so that the real error is surfaced. These are setup helpers that are expected to succeed, so ExecutionException and InterruptedException are wrapped in RuntimeException (restoring the interrupt flag) instead of being declared. - PlaintextConsumerTest#testSeek created the same topic twice. The second call used to fail silently and now throws TopicExistsException. - Remove the assertDoesNotThrow wrappers that only existed to handle the checked InterruptedException.
ClusterInstance#createTopic and #createTopicWithAssignment no longer declare checked exceptions. Remove the InterruptedException and ExecutionException declarations, and the resulting unused imports, that only existed because of them.
…pers These private helpers only forwarded to ClusterInstance#createTopic or #createTopicWithAssignment, so call the ClusterInstance methods directly.
- TopicCommandTest: inline the partition count and replication factor locals. They were also passed to waitTopicCreation before, but are now only used once in createTopic. - RemoteTopicCrudTest: drop the redundant Map.of() argument.
github-actions
Bot
added
triage
chia7712
reviewed
Oct 5, 2026
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException("Failed to create topic " + topicName, e); | ||
| } catch (ExecutionException e) { |
Member
There was a problem hiding this comment.
We can throw the cause directly if it is a subtype of RuntimeException
} catch (ExecutionException e) {
if (e.getCause() instanceof RuntimeException re) throw re;
throw new RuntimeException("Failed to create topic " + topicName, e.getCause());
}
Contributor
Author
There was a problem hiding this comment.
Thanks for pointing this out! Updated
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException("Failed to create topic " + topicName, e); | ||
| } catch (ExecutionException e) { |
chia7712
reviewed
Oct 5, 2026
| } | ||
|
|
||
| default void createTopic(String topicName, int partitions, short replicas, Map<String, String> props) throws InterruptedException { | ||
| default void createTopic(String topicName, int partitions, short replicas, Map<String, String> props) { |
Member
There was a problem hiding this comment.
BTW, we could apply the same style to deleteTopic to get rid of the checked exceptions :)
m1a2st
reviewed
Oct 6, 2026
Comment on lines
501
to
503
| private void createTopic(String name, short replicationFactor) { | ||
| clusterInstance.createTopic(name, 1, replicationFactor); | ||
| } |
m1a2st
reviewed
Oct 6, 2026
m1a2st
left a comment
m1a2st
left a comment
Contributor
There was a problem hiding this comment.
Could you also remove the throws clauses that are no longer reachable? A few were missed:
ProducerFailureHandlingTest:createInternalTopicand its two callersResetIntegrationTestTransactionsTestHelper: the five timeout /testEmptyAbortAfterCommithelper methods. Thethrows Exceptionon their call sites inTransactionsTestandTransactionsWithTieredStoreTestis redundant as well.ConfigCommandIntegrationTest,ResetConsumerGroupOffsetTest, andTopicCommandTest
Contributor
Author
|
@m1a2st Thanks! Removed the ones you listed. I also did a broader sweep with the same rule and found a few more in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to
#23513
to address the review comments.
ClusterInstance#createTopic and #createTopicWithAssignment did not wait
for the CreateTopics result. If the creation failed, the real error was
hidden by the 60 second waitTopicCreation timeout, or ignored if the
topic already existed. They now call get() so that the real error can be
propagated.
These are setup helpers that are expected to succeed, so
ExecutionException and InterruptedException are caught and callers no
longer need to declare them.
Other cleanups:
existed
because of the checked exceptions.
private helpers.
redundant Map.of() in RemoteTopicCrudTest.
duplicate call is removed.
Co-Authored by Claude
Reviewers: Chia-Ping Tsai chia7712@gmail.com, Ken Huang
s7133700@gmail.com