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

Implement static membership feature on client side (KIP-345) #1594

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

Conversation

sandeepkumarcel
Copy link

implement kip-345

add test fixtures

ts-defs

implement kip-345

add test fixtures

ts-defs
@@ -12,6 +12,8 @@ const ResourcePatternTypes = require('./src/protocol/resourcePatternTypes')
const { isRebalancing, isKafkaJSError, ...errors } = require('./src/errors')
const { LEVELS } = require('./src/loggers')

// local two

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errant comment maybe?

@distracteddev
Copy link

distracteddev commented Jul 5, 2023

This is exciting work! It seems like this has also implemented most of (all of?) KIP-320.

Can you help me understand what parts of Nevon's earlier checklist might still be left to complete?

  1. Implement KIP-320. This is probably gonna be an epic in and of itself. The reason we need this is because we need to support Fetch v12 in order to get the currentLeaderEpoch field, which we need in order to support OffsetFetch v6-7, which in turn we need because it includes the groupInstanceId field that's needed for KIP-345.
  2. Add support for OffsetFetch v7
  3. Change the behavior of a consumer leaving the group to not issue a LeaveGroup request if the instance id is set.
  4. Expose a way of configuring the instance id on the consumer.
  5. Update the partition assigner to take into account the instance id (the KIP has a good example of how)
    I have not really mapped out the work for KIP-320, so I don't know how big or small it is, but it's definitely the first step needed. We have another issue open for that Support KIP-320: Handle log truncation #818

I'd love to help get this over the line.

@Rez0k
Copy link

Rez0k commented Jul 23, 2023

can we advance this pr?
we can really use this ability

@swan2488
Copy link

This feature will be very helpful in our case.
Is there any way to speed up the pr

@abbccdda
Copy link

Same here, this is really useful.

@olfedorenk
Copy link

This pr will help us a lot

@usbtrs
Copy link

usbtrs commented Sep 14, 2023

Agree, that this feature will help many developers

@Gekeris
Copy link

Gekeris commented Sep 14, 2023

Please don't delay this pr. I need it

@larysakutyrkina
Copy link

+1, would help a lot with a current project

if (memberId) {
await this.coordinator.leaveGroup({ groupId, memberId })
await this.coordinator.leaveGroup({ groupId, memberId, groupInstanceId })

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not send leave group if static membership is enabled, i.e groupInstancId is defined.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you change it yourself? We're just waiting for this feature and no one is reacting!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abbccdda It is not necessary that anybody who uses static membership would never want to stop the graceful termination of consumption. This is a valid change. If one does not want to call leave group then just have to not call the consumer.stop() & consumer.disconnect() & leave group won't be called.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey it seems like a lot of people are waiting for this (including me). I'd be willing to take this on since it seems like it's so close to being done. What's there left to do? Just this?

@Exc1usive
Copy link

@sandeepkumarcel check please comments, realy waiting for this PR

@priyesh2609
Copy link

@sandeepkumarcel have you tested this PR somewhere? if yes, then let us know, at least I am ok to use your forked version in my use case.
Thanks 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.