-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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 rack-awareness (KIP-881) to sticky-assignor #4274
Add rack-awareness (KIP-881) to sticky-assignor #4274
Conversation
…n a rebalance is in progress and partitions are owned by consumer
da687ac
to
a1e35a9
Compare
Hi @wmorgan6796 , yep, that makes sense, @emasab discussed it internally as well. I'll add your commit from #4220 into this so it might get in as well. Thanks |
This is an excellent improvement. We also really need her. |
+1 |
It would be a great change! |
3f50a9d
to
b10cf3d
Compare
a1e35a9
to
86d22e7
Compare
* memory leak in test * test naming * test copyright
Merged this into the branch dev_kip_881_range_assignor for review. Includes @wmorgan6796 's changes. |
* [cooperative][manual commit] Prevent offsets from being committed when a rebalance is in progress and partitions are owned by consumer * Add test * Format * Ensure the operation is thread safe * Add missing test config * Don't propagate results if we decide to skip committing * fixing broken test * fixing typo * Add rack-awareness (KIP-881) to sticky-assignor * Fix minor issues with commiting only when not rebalancing * memory leak in test * test naming * test copyright * Fix style and remove rd_list_add_const --------- Co-authored-by: Roxane Fruytier <[email protected]> Co-authored-by: William Morgan <[email protected]> Co-authored-by: William Morgan <[email protected]>
* [cooperative][manual commit] Prevent offsets from being committed when a rebalance is in progress and partitions are owned by consumer * Add test * Format * Ensure the operation is thread safe * Add missing test config * Don't propagate results if we decide to skip committing * fixing broken test * fixing typo * Add rack-awareness (KIP-881) to sticky-assignor * Fix minor issues with commiting only when not rebalancing * memory leak in test * test naming * test copyright * Fix style and remove rd_list_add_const --------- Co-authored-by: Roxane Fruytier <[email protected]> Co-authored-by: William Morgan <[email protected]> Co-authored-by: William Morgan <[email protected]>
This builds on the protocol change PR to add rack awareness to the Sticky Assignor.
I understand that this is quite a large PR, here are the main changes:
Start with rd_kafka_sticky_assignor_assign_cb. They match, almost completely, with the Java changes to AbstractStickyAssignor.java in this PR.. Additionally, a struct matching RackInfo in the Java implementation, is added.
There are, what I think are two pre-existing bugs in the implementation that I found while porting tests. They are marked in the commit itself as a comment (I will remove the comment after first round of reviews).
Unit test addition: tests missing or different from AbstractStickyAssignorTest.java are added, existing tests are parametrized to include rack cases (rack consumer, rack brokers, rack both).
Some refactoring of common functions across assignors and unit tests into rdkafka_assignor.h/c to avoid duplication.