Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Round-robin partition assignments across multiple topics #216

Closed
wants to merge 1 commit into from

Conversation

pd
Copy link

@pd pd commented Feb 9, 2018

This is closer to the behavior of Kafka's Java consumer, as described
here:

https://github.com/apache/kafka/blob/15bc405/clients/src/main/java/org/apache/kafka/clients/consumer/RoundRobinAssignor.java#L31-L55

This is valuable in situations where you have highly disparate
partition counts. Given a few group members subscribed to many topics,
only some of which have more than one partition, the first member
will receive far more assignments than the others.

As an example, I've created topics t0..t9 with a single partition, then
t10 and t11 with 4 partitions. I booted two consumers compiled against
sarama-cluster master in the same group and printed out their assignments
once both had joined:

c0: map[t1:[0] t9:[0] t6:[0] t7:[0] t8:[0] t4:[0] t11:[0 2] t2:[0] t5:[0] t0:[0] t3:[0] t10:[0 2]]
c1: map[t10:[1 3] t11:[1 3]]

Using this branch instead:

c0: map[t11:[0 2] t7:[0] t4:[0] t6:[0] t0:[0] t2:[0] t10:[0 2]]
c1: map[t11:[1 3] t5:[0] t9:[0] t3:[0] t8:[0] t1:[0] t10:[1 3]]

So far, I've tried not to change too much: the existing test cases aren't
actually different, just uglier because of the more complicated API.
The additional test cases introduced match the scenario in the comment
linked above.

I'd actually like to make a bigger change, but didn't want to start down that path
without getting some feedback first. It would be nice to instead expose the
assignment strategy implementation as an interface, so users could provide
the logic. Something like:

// Dunno what the exact types would be, or if this would be better as a
// full fledged interface instead of just a func-type:
type Assignor func(subs Subscriptions) Assignments

type Config struct {
    Group struct {
        PartitionAssignor Assignor
        // Maintain backwards compatibility by falling back to the current Strategy config:
        PartitionStrategy Strategy
    }
}

This would enable users to do things like try implementing the StickyAssignor,
which is a ton of code and would probably be twice as much in Go, without needing to
fork or get the code upstream before it's been put through its paces.

Current implementation here could become this without too much additional effort,
just gotta work out what the most ergonomic types would be for the inputs and outputs
to the assignor.

This is closer to the behavior of Kafka's Java consumer, as described
here:

https://github.com/apache/kafka/blob/15bc405/clients/src/main/java/org/apache/kafka/clients/consumer/RoundRobinAssignor.java#L31-L55

This is valuable in situations where you have highly disparate
partition counts. Given three group members subscribed to many topics,
only a few of which have more than one partition, the first member
will receive far more assignments than the others.
@pd
Copy link
Author

pd commented Feb 9, 2018

Tests failures look to be due to map iteration order being undefined. AFAICT the one place this implementation does not currently match apache's is that we don't sort the topics first, so it's just whatever order the map happens to give them to us. I can either retool to start sorting topics, or reduce the specificity of tests; thoughts?

pd added a commit to pd/sarama-cluster that referenced this pull request Feb 13, 2018
This incorporates the round robin changes from bsm#216 (a64a8be) and
exposes the ability for users to implement their own partition assignor.

The tests for the two assignors now match the Java consumer's, to
validate that the behaviors match up as expected. Reviewing the
previous tests, I _think_ all of the same scenarios are still
covered, as well.

Backwards compatibility is maintained by falling back to the value in
`PartitionStrategy` if the `PartitionAssignor` is unset.
@pd
Copy link
Author

pd commented Feb 13, 2018

Closing in favor of #218.

@pd pd closed this Feb 13, 2018
pd added a commit to pd/sarama-cluster that referenced this pull request Mar 22, 2018
This incorporates the round robin changes from bsm#216 (a64a8be) and
exposes the ability for users to implement their own partition assignor.

The tests for the two assignors now match the Java consumer's, to
validate that the behaviors match up as expected. Reviewing the
previous tests, I _think_ all of the same scenarios are still
covered, as well.

Backwards compatibility is maintained by falling back to the value in
`PartitionStrategy` if the `PartitionAssignor` is unset.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant