Unable to connect to any cluster with any topic having a partition leader of -1

[wizzat]

You are unable to connect to any cluster with any topic having a partition leader of -1. In practice this might happen when you pull a machine out of the cluster that is the sole container for an unreplicated partition.

I've patched my local copy to ignore partitions that have a leader of -1, but this seems pretty hacky. I'll create a pull request if this is a reasonably safe way to go about things.

[rdiomar]

The only place I see this being an issue is in KafkaClient._send_broker_aware_request(), where it raises a PartitionUnavailableError if the leader is -1. I don't think you want to ignore it. To be safe, you either want to send all payloads or none of them, so raising an exception is valid. Maybe the right course of action is to catch that exception and retry after giving kafka a chance to allocate a new leader.

[wizzat]

It actually throws a KeyError on line kafka/client.py:247 (load_metadata_for_topics) and prevents connection to the cluster at all. And just to be clear: Kafka will never allocate a new leader for that partition because it was created without replication and only ever existed on a machine that was removed from the cluster.

It seems like a metadata problem with a topic/partition I'm not looking at should not prevent connection to the cluster at all.

[wizzat]

bin $ python
Python 2.7.6 (default, Dec 21 2013, 20:39:20)
[GCC 4.2.1 Compatible Apple LLVM 5.0 (clang-500.2.79)] on darwin
Type "help", "copyright", "credits" or "license" for more information.

import kafka
c = kafka.client.KafkaClient(.....)
Traceback (most recent call last):
File "", line 1, in
File "build/bdist.macosx-10.9-x86_64/egg/kafka/client.py", line 38, in init
File "build/bdist.macosx-10.9-x86_64/egg/kafka/client.py", line 247, in load_metadata_for_topics
KeyError: -1

[rdiomar]

Ah I see. You're right, this error should be ignored. These are the issues I see with handling unassigned leaders:

• load_metadata_for_topics() should put None in self.topics_to_brokers for unassigned leaders
• _get_leader_for_partition() should check for None and reload metadata if necessary.
• _send_broker_aware_request() should check for None instead of -1 since _get_leader_for_partition() returns a BrokerMetadata instance, not an ID.

If you would like to handle this and submit a pull request with unit tests, that would be great! If not then I can do it some time next week.

[mrtheb]

I think the only thing missing is a check for meta.leader != -1 before the assigment to topics_to_brokers. I missed that in the review for #100.

If you look in test_unit.py, there are already some unit tests for this case that are marked as skip because I knew the bug and wanted to fix it. The implementation has slightly changed since then but it should be easy to arrange.

[mrtheb]

Please see if #109 fixes the issue you are seeing.

[mrtheb]

@rdiomar Going through what you suggested before, to gather feedback on my PR.

• load_metadata_for_topics does not put any unreliable information in topics_to_brokers (i.e. no None to check)
• _get_leader_for_partition already reloads metadata if necessary and raises KafkaRequestError when a TopicAndPartition is not available (no need to check for None there)
• _send_broker_aware_request checks for -1 and raises PartitionUnavailableError but that condition is useless because _get_leader_for_partition raised an exception before that check.

Based on my PR, I would improve by removing the check for leader == -1 in _send_broker_aware_request and give more precision in the KafkaRequestError. The only downside of this is, if someone sends a request for multiple topics/partitions, it will fail if only one of them has a problem. I think it can be handled above.

Let me know what you think (do yo still think checking for None is the right option?). I'll update accordingly.

[rdiomar]

@mrtheb Maybe I should have stated my reasoning for the changes I suggested. I wanted to differentiate between the case where the partition has no leader, and the partition does not exist. load_metadata_for_topics would put None in topics_to_brokers to indicate that the topic partition exists, but no leader is assigned, so we can raise a different error. It's probably not necessary since in both cases we would want to reload metadata (either to check if the partition got a leader or if it was created), just more accurate. The error that _get_leader_for_partition raises says "Partition does not exist", which is not necessarily true.

[mrtheb]

I updated the PR according to your comment. After working a bit more around the unit tests, I think using None when leader is unavailable makes sense. I am raising the same error though cause I don't think it is necessary to have this level of details at that point.

Let me know.

[dpkp]

#109 merged -- should fix this issue. re-open if not!