Skip to content
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

feat(clients): MemberToRemove accepting memberId field on constructor… #18738

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

asorian0
Copy link

@asorian0 asorian0 commented Jan 29, 2025

… so it is used to build up the MemberIdentity, making it possible to use removeMembersFromConsumerGroup method from admin client to disconnect a member (denoted by memberId) from the given consumer group (denoted by groupInstanceId)

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

This change allows to use admin client's removeMembersFromConsumerGroup pointing to an actual consumer group member

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Unit test checking both options for MemberIdentity generated from MemberToRemove if holding memberId or not

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

… so it is used to build up the MemberIdentity, making it possible to use removeMembersFromConsumerGroup method from admin client to disconnect a member (denoted by memberId) from the given consumer group (denoted by groupInstanceId)
@github-actions github-actions bot added triage PRs from the community clients small Small PRs labels Jan 29, 2025
@AndrewJSchofield
Copy link
Member

@asorian0 Thanks for the PR. This proposes a change to the public API (because it changes MemberToRemove) which means that it would require a very small KIP to get approval to make the change.

I am not a complete expert on this but I would say that it is intentional to provide the group instance ID. This signifies a static group member which means that it can leave and rejoin without losing its place in the group, essentially minimising churn when the member restarts. As a result, you might want to remove the static member administratively if you know it's stopped permanently.

You could start a discussion on the Kafka dev mailing list to see whether there's interest in making the change to permit dynamic group members to be removed in the same way. Alternatively, you could create a KIP and start the discussion that way.

Hope this helps.

@AndrewJSchofield AndrewJSchofield added kip Requires or implements a KIP and removed triage PRs from the community labels Jan 29, 2025
@asorian0
Copy link
Author

Thanks @AndrewJSchofield. I applied yesterday through ASF explaining the case and actually this PR was created as part of the explanation cause guess it is way faster to see the changes to understand the point.

The context we're using kafka consumers is in k8s pods, so we are forced to use dynamic members for sure in order to guarantee the proper scalation of systems. Guess we're in a pretty common scenario these days and would make sense to prepare kafka for it.

Will wait for Apache reply about ASF application and see how to proceed after its resolution.

@AndrewJSchofield
Copy link
Member

Just in case you've not found them yet, I would read https://kafka.apache.org/contributing and https://kafka.apache.org/contact. If you join the developer mailing list, you can start the discussion of this subject on there. That's where I would start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clients kip Requires or implements a KIP small Small PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants