KIP-54: Implement sticky partition assignment strategy #2057

aynroot · 2020-05-26T15:06:18Z

Implementation of a sticky partition assignment strategy for kafka consumers.

General motivation behind sticky assignment:

In certain circumstances the round robin assignor, [...], fails to produce an optimal and balanced assignment of topic partitions to consumers. [...] In addition, when a reassignment occurs, none of the existing strategies consider what topic partition assignments were before reassignment, as if they are about to perform a fresh assignment. Preserving the existing assignments could reduce some of the overheads of a reassignment. For example, Kafka consumers retain pre-fetched messages for partitions assigned to them before a reassignment. Therefore, preserving the partition assignment could save on the number of messages delivered to consumers after a reassignment. Another advantage would be reducing the need to cleanup local partition state between rebalances.

The assignment algorithm is described here: KIP-54
An implementation of the strategy in Java: AbstractStickyAssignor.java + StickyAssignor.java. And its tests: AbstractStickyAssignorTest.java + StickyAssignorTest.java

This change is

…support

… items()

…variables

aynroot · 2020-05-28T09:34:14Z

CI checks are failing for Python 3.7 due to a pylint issue, looks like it is being addressed already here #2058

tvoinarovskyi · 2020-05-28T13:45:30Z

@aynroot Yea, you can ignore that CI error for now, I need to debug it properly

aynroot · 2020-05-28T14:30:31Z

@tvoinarovskyi alright, thanks for the info
then this PR is ready for review

dpkp · 2020-09-07T23:37:33Z

kafka/coordinator/assignors/abstract.py

@@ -44,13 +44,14 @@ def metadata(self, topics):
        pass

    @abc.abstractmethod
-    def on_assignment(self, assignment):
+    def on_assignment(self, assignment, generation):


This is a breaking change for anyone who may have implemented their own custom assignor, as they will now need to add a generation parameter. Is there a way to structure this so that it doesn't require changes to existing assignors?

I haven't looked deeply into KIP-429, but if it also leverages generations (and I suspect it might given that it includes incremental shifts), then we may want to consider keeping this and bumping the kafka-python version to 3.x for the breaking change... I have a feeling it may make our lives easier down the road. But if they don't need it there, then agree it would be nicer not to add it just for the sticky assignor case...

Not sure about KIP-429, but generations here are crucial for KIP-341 bug fix.

We can allow backwards compatibility with something like this in consumer.py

assignor.on_assignment(assignment) if assignor.name == 'sticky': assignor.on_generation_assignment(generation)

I'll make necessary changes.

dpkp · 2020-09-07T23:38:22Z

kafka/coordinator/consumer.py

@@ -6,6 +6,7 @@
 import logging
 import time

+from kafka.coordinator.assignors.sticky.sticky_assignor import StickyPartitionAssignor


import order

dpkp · 2020-09-07T23:40:14Z

kafka/coordinator/consumer.py

@@ -31,7 +32,7 @@ class ConsumerCoordinator(BaseCoordinator):
        'enable_auto_commit': True,
        'auto_commit_interval_ms': 5000,
        'default_offset_commit_callback': None,
-        'assignors': (RangePartitionAssignor, RoundRobinPartitionAssignor),
+        'assignors': (RangePartitionAssignor, RoundRobinPartitionAssignor, StickyPartitionAssignor),


would rather not add this to the default list, especially since it requires an external dependency

This assignment algorithm isn't just sticky, it's also better balanced than either of the other two... and if there's a choice between sticky vs balanced, it favors balanced... at least that's what I recall from when I helped drive that KIP forward.

Given that, it'd be nice to add to the default list, and I'd even support moving it up the list to the primary default so that users get the most optimized balancing algorithm. The only risk is if the implementation is buggy, but it looks like there's a solid test suite here.

Be good to know what the Java client does here, I'd check but my wife just said dinner is ready so I need to roll...

I looked into this a little more, and based on the work in KIP-429, it's probably not a great idea to set this as the preferred option...

But I do still support including it in the list of default options. Additionally, if this is joining a mixed-language consumer group, it's more likely that it'd be cross-compatible with the Java clients etc if we include this in the default list.

It also makes it discoverable to folks that it's an option... because otherwise it's not obvious in the main consumer docs that this is an option.

dpkp · 2020-09-07T23:48:07Z

setup.py

@@ -58,5 +59,8 @@ def run(cls):
        "Programming Language :: Python :: 3.7",
        "Programming Language :: Python :: Implementation :: PyPy",
        "Topic :: Software Development :: Libraries :: Python Modules",
+    ],
+    install_requires=[
+        "sortedcontainers==2.1.0"


kafka-python has to-date not taken on any external dependencies, and we'd like to keep with that philosophy. Is this module necessary / can we refactor to use the standard library instead? If it is necessary, can we make it optional and disable the assignor if it is not installed?

It'd be really nice to see this land without an external dependency...

Yep, thought about it as well, should not be a rocket science to implement sorted sets by hand. Will do.

dpkp · 2020-09-07T23:49:48Z

kafka/coordinator/consumer.py

@@ -233,7 +234,7 @@ def _on_join_complete(self, generation, member_id, protocol,

        # give the assignor a chance to update internal state
        # based on the received assignment
-        assignor.on_assignment(assignment)
+        assignor.on_assignment(assignment, generation)


this will also crash if the user has configured a custom assignor that doesn't take the additional parameter

dpkp · 2020-09-07T23:51:26Z

Thanks -- I can see that you've put in a lot of work on this, and we'd love to get it merged. I've left a few comments re: compatibility and dependencies that I would like to resolve before adding to kafka-python.

jeffwidman · 2020-09-17T06:35:18Z

Does this also include the updates described in KIP-341? I'm sure Vahid also increased the test coverage to cover this case...

If you ported the latest Java code, you should be fine, but if you based your implementation off the original KIP-54 description, then we'd want to include the KIP-341 bugfix...

aynroot · 2020-09-22T12:18:15Z

Does this also include the updates described in KIP-341? I'm sure Vahid also increased the test coverage to cover this case...

If you ported the latest Java code, you should be fine, but if you based your implementation off the original KIP-54 description, then we'd want to include the KIP-341 bugfix...

Yep, it includes KIP-341

aynroot · 2020-09-29T08:53:09Z

@dpkp @jeffwidman I've addressed all the comments, this PR is ready to be re-reviewed.
Please take a look when you have time.

dpkp

Thanks, this is great and a huge new feature!! Fantastic work

sibiryakov · 2020-10-09T15:01:51Z

@aynroot Well done Valeria!

aynroot force-pushed the KIP-54-sticky-assignor branch 2 times, most recently from 5611809 to fc9d90f Compare May 27, 2020 12:45

aynroot added 5 commits May 27, 2020 15:02

[KIP-54] Implement sticky partition assignment strategy without py27 …

650e81e

…support

[KIP-54] Remove python3 typing hints

c914985

[KIP-54] Use six instead of direct usages of for keys(), values() and…

32f35be

… items()

[KIP-54] f-strings -> string.format

d16d91f

[KIP-54] Disable no-member and not-an-iterable checks for assignment …

6d5e612

…variables

aynroot force-pushed the KIP-54-sticky-assignor branch from fc9d90f to 6d5e612 Compare May 27, 2020 13:03

aynroot marked this pull request as ready for review May 27, 2020 13:24

dpkp reviewed Sep 7, 2020

View reviewed changes

aynroot added 2 commits September 28, 2020 13:21

[KIP-54] Split on_assignment method to allow backwards compatibility.

ae5f2ae

[KIP-54] Fix import order

53cc810

aynroot force-pushed the KIP-54-sticky-assignor branch 3 times, most recently from 539b6f4 to e4c05f8 Compare September 28, 2020 14:29

aynroot requested review from jeffwidman and dpkp September 29, 2020 08:52

[KIP-54] Remove extra dependency on sortedcontainers

94ad7a3

aynroot force-pushed the KIP-54-sticky-assignor branch from e4c05f8 to 94ad7a3 Compare September 29, 2020 08:54

dpkp approved these changes Sep 30, 2020

View reviewed changes

dpkp merged commit c536dd2 into dpkp:master Sep 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KIP-54: Implement sticky partition assignment strategy #2057

KIP-54: Implement sticky partition assignment strategy #2057

aynroot commented May 26, 2020 •

edited

Loading

aynroot commented May 28, 2020

tvoinarovskyi commented May 28, 2020

aynroot commented May 28, 2020

dpkp Sep 7, 2020

jeffwidman Sep 17, 2020

aynroot Sep 22, 2020

dpkp Sep 7, 2020

dpkp Sep 7, 2020

jeffwidman Sep 17, 2020 •

edited

Loading

jeffwidman Sep 17, 2020 •

edited

Loading

dpkp Sep 7, 2020

jeffwidman Sep 17, 2020 •

edited

Loading

aynroot Sep 22, 2020

dpkp Sep 7, 2020

dpkp commented Sep 7, 2020

jeffwidman commented Sep 17, 2020 •

edited

Loading

aynroot commented Sep 22, 2020

aynroot commented Sep 29, 2020

dpkp left a comment

sibiryakov commented Oct 9, 2020

KIP-54: Implement sticky partition assignment strategy #2057

KIP-54: Implement sticky partition assignment strategy #2057

Conversation

aynroot commented May 26, 2020 • edited Loading

aynroot commented May 28, 2020

tvoinarovskyi commented May 28, 2020

aynroot commented May 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffwidman Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

jeffwidman Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffwidman Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpkp commented Sep 7, 2020

jeffwidman commented Sep 17, 2020 • edited Loading

aynroot commented Sep 22, 2020

aynroot commented Sep 29, 2020

dpkp left a comment

Choose a reason for hiding this comment

sibiryakov commented Oct 9, 2020

aynroot commented May 26, 2020 •

edited

Loading

jeffwidman Sep 17, 2020 •

edited

Loading

jeffwidman Sep 17, 2020 •

edited

Loading

jeffwidman Sep 17, 2020 •

edited

Loading

jeffwidman commented Sep 17, 2020 •

edited

Loading