-
Notifications
You must be signed in to change notification settings - Fork 14.8k
MINOR: Improve endpoint information epoch management in Streams group coordinator #20854
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
Conversation
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.
Pull Request Overview
This PR refactors endpoint information management in Streams group heartbeat responses by changing the initial endpoint information epoch from -1 to 0 and improving when endpoint information is included in responses. The key changes aim to ensure that endpoint information is properly included when members join and that the epoch handling is more intuitive.
Key Changes:
- Changed the default endpoint information epoch from -1 to 0 in StreamsGroup
- Modified logic to always return endpoint information for newly created groups (epoch 0)
- Simplified maybeBuildEndpointToPartitions to always return a list and properly include the updated member
- Added a new test case to verify endpoint information includes all members when a new member joins
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| GroupMetadataManagerTest.java | Updated test expectations to remove explicit endpoint epoch settings and added new test for multi-member endpoint scenarios |
| EndpointToPartitionsManager.java | Added new helper method maybeEndpointToPartitions to create endpoint mappings only when members have endpoints |
| StreamsGroup.java | Changed default endpointInformationEpoch from -1 to 0 |
| GroupMetadataManager.java | Modified endpoint epoch response logic and refactored endpoint list building to properly include updated members |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java
Outdated
Show resolved
Hide resolved
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java
Show resolved
Hide resolved
…roup/GroupMetadataManager.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
bbejeck
left a comment
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 PR @lucasbru LGTM
… coordinator (apache#20854) Fix endpoint information epoch management in Streams group coordinator This commit addresses several issues in the endpoint information epoch handling for Streams groups: 1. Conditional epoch bumping: Only increment endpointInformationEpoch when a member actually has a user endpoint defined, rather than on every member update. This prevents unnecessary epoch bumps for members without endpoints. 2. Initial epoch value: Change endpointInformationEpoch initialization from -1 to 0 to be consistent with standard epoch semantics. 3. Response epoch handling: Only include endpointInformationEpoch in responses when the group has been persisted, to better deal with the case where a group is newly created but the epoch information is not persisted (because it's soft state which is thrown away after the execution of the first heartbeat). 4. Endpoint-to-partitions building: Refactor maybeBuildEndpointToPartitions to consistently include both existing and new members' endpoint information. The previous implementation did not incldue the endpoint for a new member. Also return an empty list instead of null if the set of endpoints actually becomes empty. The changes ensure that endpoint information is correctly propagated to all group members during rebalancing, including when new members join with user endpoints. Tests updated to reflect the new behavior and a comprehensive test added to verify endpoint information includes both existing and new members. Reviewers: Bill Bejeck<bbejeck@apache.org> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… coordinator (apache#20854) Fix endpoint information epoch management in Streams group coordinator This commit addresses several issues in the endpoint information epoch handling for Streams groups: 1. Conditional epoch bumping: Only increment endpointInformationEpoch when a member actually has a user endpoint defined, rather than on every member update. This prevents unnecessary epoch bumps for members without endpoints. 2. Initial epoch value: Change endpointInformationEpoch initialization from -1 to 0 to be consistent with standard epoch semantics. 3. Response epoch handling: Only include endpointInformationEpoch in responses when the group has been persisted, to better deal with the case where a group is newly created but the epoch information is not persisted (because it's soft state which is thrown away after the execution of the first heartbeat). 4. Endpoint-to-partitions building: Refactor maybeBuildEndpointToPartitions to consistently include both existing and new members' endpoint information. The previous implementation did not incldue the endpoint for a new member. Also return an empty list instead of null if the set of endpoints actually becomes empty. The changes ensure that endpoint information is correctly propagated to all group members during rebalancing, including when new members join with user endpoints. Tests updated to reflect the new behavior and a comprehensive test added to verify endpoint information includes both existing and new members. Reviewers: Bill Bejeck<bbejeck@apache.org> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fix endpoint information epoch management in Streams group coordinator
This commit addresses several issues in the endpoint information epoch
handling for Streams groups:
Conditional epoch bumping: Only increment endpointInformationEpoch
when a member actually has a user endpoint defined, rather than on
every member update. This prevents unnecessary epoch bumps for
members without endpoints.
Initial epoch value: Change endpointInformationEpoch initialization
from -1 to 0 to be consistent with standard epoch semantics.
Response epoch handling: Only include endpointInformationEpoch in
responses when the group has been persisted, to better deal with the
case where a group is newly created but the epoch information is
not persisted (because it's soft state which is thrown away after
the execution of the first heartbeat).
Endpoint-to-partitions building: Refactor
maybeBuildEndpointToPartitions
to consistently include both existing and new members' endpoint
information. The previous implementation did not incldue the endpoint
for a new member. Also return an empty list instead of null if the
set of endpoints actually becomes empty.
The changes ensure that endpoint information is correctly propagated to
all group members during rebalancing, including when new members join
with user endpoints. Tests updated to reflect the new behavior and a
comprehensive test added to verify endpoint information includes both
existing and new members.
Reviewers: Bill Bejeckbbejeck@apache.org