search

dpkp / kafka-python

Python client for Apache Kafka
http://kafka-python.readthedocs.io/
Apache License 2.0
5.61k stars 1.41k forks source link

Breaking change/bug in ClusterMetadata #1934

Open ofek opened 5 years ago

ofek commented 5 years ago

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 commented 5 years ago

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

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 commented 4 years ago

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 commented 4 years ago

We have logic where we do if coord_id >= 0

dpkp commented 4 years ago

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).