-
Notifications
You must be signed in to change notification settings - Fork 132
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
[FLINK-36210] Optimize the logic for fetching topic metadata in the TopicPatternSubscriber mode #117
Conversation
static Map<String, TopicDescription> getTopicMetadata(AdminClient adminClient, Pattern topicPattern) { | ||
try { | ||
Set<String> allTopicNames = adminClient.listTopics().names().get(); | ||
Set<String> patternTopicNames = allTopicNames.stream() |
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.
nit: matchedTopic* would sound nicer than patternTopic* (not just here, in the whole pr)
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.
nit: matchedTopic* would sound nicer than patternTopic* (not just here, in the whole pr)
Thank you for your careful review, I changed to them.
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.
nit: matchedTopic* would sound nicer than patternTopic* (not just here, in the whole pr)
done.
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.
thx
Friendly ping, do you have time to take a look @AHeise 🙏 ? |
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'll approve and merge once CI passes.
Have you seen any performance improvements with this change? Could you add that to the description? I'm assuming you are submitting the change because the old code took too long to recover.
Don't forget to run spotless. |
Thanks for the reminder! |
done. |
Awesome work, congrats on your first merged pull request! |
Merged, thank you for your contribution :) |
What is the purpose of the change
Optimize the logic for fetching topic metadata in the TopicPatternSubscriber mode to speed up task recovery.
Brief change log
In
TopicPatternSubscriber
mode, our current logic for fetch topic metadata for all topics and then filtering it. We can optimize this by first filtering the topic names and then fetch metadata only for the filtered topics.Verifying this change
Documentation