Breaking change/bug in ClusterMetadata

[ofek]

coordinator_for_group should return an int, but now may return a str.

https://github.com/dpkp/kafka-python/blob/1.4.7/kafka/cluster.py#L165

add_group_coordinator:

https://github.com/dpkp/kafka-python/blob/1.4.7/kafka/cluster.py#L360-L369

cc @dpkp @jeffwidman

[jeffwidman]

Oops. And the docs even show they have different contracts:

• https://github.com/dpkp/kafka-python/blob/1.4.7/kafka/cluster.py#L162
• https://github.com/dpkp/kafka-python/blob/1.4.7/kafka/cluster.py#L349

IMO they should be internally consistent, but I'm not sure if the "fix" is to make them both strings or ints.

For backwards compatibility it should be int, but our next release is likely to be a 2.0 due to #1196 / #1816 in which case we have the freedom here to make a breaking change.

I'm not eager to make breaking changes, but would be willing to do so if Java and the Kafka spec, etc allow for string node IDs...

[dpkp]

The node_id should be treated as opaque. Is there something special about the prior int-ness ? We could fake an integer, but because this is python it seems like a more descriptive key makes more sense. Can you describe the problem this is causing for your code?

[ofek]

We have logic where we do if coord_id >= 0

[dpkp]

Ok -- that's understandable. It looks like you are trying to filter out or identify error cases. These could be better documented: None indicates that the group is unknown. -1 indicates that the group is known but the coordinator metadata is not available (error). Your code might be better served by checking these error cases explicitly. This will both improve your handling of None, which I think would crash your current filter, as well as the string node ids used for coordinators (also used for bootstrap nodes).

[dpkp]

Updated docs in master to make this a bit more clear.