-
Notifications
You must be signed in to change notification settings - Fork 140
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-35228][Connectors/Kafka] Fix: DynamicKafkaSource does not read re-added topic for the same cluster #97
Conversation
Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html) |
Sorry, I forgot to change issue number while copying from another PR as a template. Fixed now. |
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 contribution! The issue you are seeing and integration test make sense to me but I don't understand how you have fixed it. The changes to DynamicKafkaSourceEnumerator looks like solely refactoring. Did you forget to commit the fix?
The fix is that previously And yes, while doing that some refactoring came in naturally to avoid code duplication. |
Thanks for explaining, I missed that! Rerunning CI, the PR LGTM |
...org/apache/flink/connector/kafka/dynamic/source/enumerator/DynamicKafkaSourceEnumerator.java
Outdated
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 one comment. Can you also fixup the PR title? e.g. your jira ticket title is a bit more informative: "DynamicKafkaSource does not read re-added topic for the same cluster"
Changed the title as requested. Also, I've added filtering to |
...apache/flink/connector/kafka/dynamic/source/enumerator/DynamicKafkaSourceEnumeratorTest.java
Outdated
Show resolved
Hide resolved
Rerunning CI--seems to be flaky behavior in IT tests:
|
...apache/flink/connector/kafka/dynamic/source/enumerator/DynamicKafkaSourceEnumeratorTest.java
Outdated
Show resolved
Hide resolved
… re-added topic for the same cluster
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 iterations! LGTM
Awesome work, congrats on your first merged pull request! |
Saw some dormant code that was collecting
activeTopicPartitions
which got used nowhere. I believe the cleanup was already intended by the original developer, but just got lost for some reason.So I've finished up the cleanup logic and added a test case to cover the issue in question.