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

Add cooperative rebalance support #907

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thynson
Copy link

@thynson thynson commented Aug 11, 2021

No description provided.

@thynson
Copy link
Author

thynson commented Aug 11, 2021

incremetalAssign and incrementalUnassign are already exposed to Node, and anyone can call them in a rebalance callback when partition.assignment.strategy is cooperative-sticky.
Does the KafkaConsumer need to be also modified to provide a default rebalance_cb for cooperative-sticky?

@thynson thynson force-pushed the cooperative-rebalance-support branch from ce559e0 to 21a2226 Compare August 12, 2021 06:40
@stale
Copy link

stale bot commented Jan 8, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issues label Jan 8, 2022
@stale stale bot closed this Mar 2, 2022
@johncsnyder
Copy link

johncsnyder commented Mar 5, 2022

I would love to see this feature added. 🙏

As far as I can tell, the cooperative-sticky partition assignment strategy can't work without adding the bindings for incremental assign/unassign.

I've tried but always received the error application *assign() call failed: Changes to the current assignment must be made using incremental_assign() or incremental_unassign() when rebalance protocol type is COOPERATIVE.

@thynson are there any plans to re-open this PR?

@thynson
Copy link
Author

thynson commented Mar 5, 2022

As far as I can tell, the cooperative-sticky partition assignment strategy can't work without adding the bindings for incremental assign/unassign.

They were implemented in this PR, 21a2226#diff-e574f6617d5a1bd9fb35fab6b02e906b120024e0dcae87b010eed716e23820d4R45, but you need to set partition.assignment.strategy to cooperative-sticky. Or if you're using custom rebalance_cb, call incrementalAssign and incrementalUnassign.

@johncsnyder
Copy link

@thynson Yeah, I saw, looks great.

Any chance this will be merged into master? I'd prefer not to maintain my own fork.

@thynson
Copy link
Author

thynson commented Mar 7, 2022

Any chance this will be merged into master? I'd prefer not to maintain my own fork.

If @iradul has time to take a look.

@iradul iradul reopened this Mar 7, 2022
@stale stale bot removed the stale Stale issues label Mar 7, 2022
@johncsnyder
Copy link

Sweet, thanks 🙏

@iradul
Copy link
Collaborator

iradul commented Mar 21, 2022

@thynson, thank you for the PR

Copy link
Collaborator

@iradul iradul left a comment

Choose a reason for hiding this comment

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

Missing items:

  • assignment_lost implementation
  • e2e test(s)

lib/kafka-consumer.js Outdated Show resolved Hide resolved
lib/kafka-consumer.js Outdated Show resolved Hide resolved
lib/kafka-consumer.js Show resolved Hide resolved
@thynson thynson force-pushed the cooperative-rebalance-support branch from 21a2226 to d55d790 Compare March 21, 2022 11:19
@thynson
Copy link
Author

thynson commented Mar 22, 2022

I'll try to implement the missing parts, but I'm still busy for some days.

@micheleangioni
Copy link

Any hope this will get implemented? @thynson :)
It would be extremely useful for us

@ZhukovAntonPls
Copy link

@micheleangioni
Did you find a way to solve this? We also really need it @thynson )

@spalax
Copy link

spalax commented May 12, 2023

and we need it a lot :)

@serj026
Copy link

serj026 commented May 12, 2023

We are eagerly awaiting this feature 🙏

aleksandrAndrosov

This comment was marked as outdated.

@notgosu
Copy link

notgosu commented May 12, 2023

we need it also :)

@votar1408
Copy link

It can save the world, great the feature! @micheleangioni

@yaroslav-fedyshyn-playson

Thanks for the PR. It will significantly reduce our struggling and help us!

@thynson thynson force-pushed the cooperative-rebalance-support branch from d55d790 to 85193d8 Compare May 12, 2023 10:19
@thynson thynson force-pushed the cooperative-rebalance-support branch from 85193d8 to 3c8e0dd Compare May 12, 2023 10:21
@thynson
Copy link
Author

thynson commented May 12, 2023

I did not have any experience of writing an e2e test for rdkafka. So any help would be great, or I have to take some time to finish the e2e test.

@ofekdeitch-oligo
Copy link

Hi @thynson , why can't we use the e2e tests that @serj026 implemented in his PR?
https://github.com/Blizzard/node-rdkafka/pull/1081/files

Maybe we can merge the two PRs and get this feature finally merged

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.