-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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-18311: Configuring changelog topics (2/N) #18379
base: trunk
Are you sure you want to change the base?
Conversation
@cadonna could you have a look? |
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 PR, @lucasbru !
Here my feedback.
/** | ||
* This class is responsible for setting up the changelog topics for a topology. | ||
*/ |
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 some more details?
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
private final Collection<Subtopology> subtopologies; | ||
private final Function<String, Integer> topicPartitionCountProvider; | ||
|
||
public ChangelogTopics( |
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 javadocs?
Especially for the topicPartitionCountProvider
.
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
|
||
private int getPartitionCountOrFail(String topic) { | ||
final Integer topicPartitionCount = topicPartitionCountProvider.apply(topic); | ||
if (topicPartitionCount == null) { |
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.
Should the topicPartitionCountProvider
return an Optional
instead of an Integer
?
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
4cc3b4b
to
acb8145
Compare
A simplified port of "ChangelogTopics" from the client-side to the group coordinator
Compared to the client-side version, the implementation uses immutable data structures, and returns the computed number of partitions instead of modifying mutable data structures and calling the admin client.
Stacked PR, only review last commit.
Committer Checklist (excluded from commit message)