Provide the _subscribed_ topics to the protocol() function

[ankon]

The topics field may change depending on the actually assigned topics, and we might effectively
lose topics here when the assigner tries to build a full view of the topics used by the members.

See also https://kafkajs.slack.com/archives/CF6RFPF6K/p1572986319131500

--
This is possibly related to changes in #226.

[JaapRood]

Very nice find! Just for my understanding: this would only be an issue when the consumer group doesn't get any partitions assigned for a subscribed topic, right?

[ankon]

Right now the assumption is that all members of a consumer group (eventually) will have exactly the same subscriptions. Because of that assumption it is fair for the round-robin-assigner to simply distribute its own topics (see

kafkajs/src/consumer/consumerGroup.js

Line 140 in 34824cb

assignment = await assigner.assign({ members, topics: topicsSubscribed })

).
This does lead to the "I'm assigned unsubscribed topics! Help!" warning though when members of the consumer group are subscribed to different topics, for example when they are starting up.

What I'm going to do in our own assigner is to avoid this problem by looking at the member metadata topics, and then assigning the combined set of topics, taking into account who as subscribed to what topic.

So, just to be clear: Right now this is not a problem for KafkaJS as such, but only when you're building an assigner that needs to understand what topics are subscribed to by each member in the group. And then this is only a problem if the assigner ever assigned a topic not subscribed -- in that case KafkaJS will remove it from topics, and the next time you're going to look at the member metadata this topic would be "gone".

[tulios]

It makes sense @ankon, thanks for the PR. We have been a bit busy these previous weeks, but I will go over all PRs next week. This one is quite small, but I want to run some things before I merge.

Thanks again!

[JaapRood]

@tulios just completely out of curiosity: what kind of things would you want to run? If that's are important, would we benefit from automating that as part of tests?

[tulios]

@JaapRood I just want to be a good maintainer and review this PR properly, I want to understand the impact of the change. I want to look at all the code around the assigner and see if can spot any issues going forward.

[JaapRood]

@tulios thanks for explaining 👍

[ankon]

Mostly for me, probably it was obvious: If you are writing an assigner that relies on the topics in the metadata, you need to make sure to provide an explicit assignment for each member topic in the assign response. Failing to do so will drop out the unassigned topics, and the next time the assigner is called you will be missing topics.

In our specific case that happened when services where restarting, and we consistently "lost" topics that were assigned to the member that just went away.

[ankon]

@tulios any update here, something I could help with?

[Nevon]

This tiny PR should not have been sitting here this long. I'm not sure I fully grasp the implications of this, but intuitively it makes sense that you want to join the group for all the topics you are subscribed to, not the topics you happen to be assigned, since there's no guarantee that you'll be assigned partitions for all topics, and as we've seen you can be assigned topics you're not subscribed to.

[ankon]

Friendly ping: I think this hasn't hit master yet, has it?

[Nevon]

🙈

I think I merged master into it and was planning on merging as soon as I got a green build, but then forgot about it. The pipeline is a lot more stable now, after I fixed some issues with it, so it shouldn't be a problem now.

[Nevon]

Sorry for the insane delay. This will be out in 1.13.0-beta.9

[ankon]

Thanks! :)