-
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-36209] Remove redundant operations in the initialization of KafkaSourceEnumState #116
Conversation
Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html) |
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.
Thank you very much for your contribution. The change almost LGTM with one little change. I'm running CI.
private static KafkaSourceEnumState deserializeTopicPartitions(byte[] serializedTopicPartitions, | ||
AssignmentStatus status) |
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.
I'd rename to deserializeAssignedPartitions
and inline status
. I don't see it being used with UNASSIGNED
.
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.
I'd rename to
deserializeAssignedPartitions
and inlinestatus
. I don't see it being used withUNASSIGNED
.
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.
I'd rename to
deserializeAssignedPartitions
and inlinestatus
. I don't see it being used withUNASSIGNED
.
done.
Friendly ping, passed ci. If you have time, pls help to review it again. thanks a lot. @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. Thank you very much for your contribution. Will merge now.
What is the purpose of the change
In certain methods, such as the
DynamicKafkaSourceEnumerator#onHandleSubscribedStreamsFetch()
method, partitions are divided intoassignedPartitions
andunassignedInitialPartitions
before being passed as parameters to the KafkaSourceEnumState constructor. However, within the constructor, theseassignedPartitions
andunassignedInitialPartitions
are recombined into partitions, leading to unnecessary operations and reduced performance. By optimizing the code to pass partitions directly as a parameter when initializing KafkaSourceEnumState, we can eliminate redundant operations and enhance performance.Brief change log
Verifying this change
Documentation