TopicAndPartition fix when partition leader = -1 #109
Conversation
|
👍 this was my bad, thanks for adding a test for it and fixing the issue. |
| if key not in self.topics_to_brokers: | ||
| # reload metadata whether the partition is not available | ||
| # or has no leader (broker is None) | ||
| if self.topics_to_brokers.get(key) is None: |
There was a problem hiding this comment.
This can be if key not in self.topics_to_brokers:
There was a problem hiding this comment.
No because I want to handle cases where key is not in dict and when the value at key is None. Get returns None by default in both cases which suits my need.
Should I clarify the comment because that is what it is supposed to say.
|
Is this going to be merged back to the 0.8.0 release branch? |
|
The next release branch will be 0.9.0 when it's ready. This will be merged into master before that branch is created. |
|
Is the 0.9.0 release going to support 0.8.0? Kafka 0.8.0 is the current stable release. |
|
Yup. We decided to divorce kafka-python's version number from kafka's. The README should always tell you what server versions are supported. |
|
Right, my point is that the current README shows kafka-python (compatible with 0.8.1, 0.9.0) is not compatible with the current stable version of Kafka (0.8.0). |
|
Ah. The truth is that kafka 0.8.0 is supported, except kafka-python has offset fetch/commit logic that kafka 0.8.0 doesn't support, so things will break if you use it. I believe it should be compatible otherwise. |
…ror for clarity
|
@rdiomar (or anyone else interested) let me know what you think of my latest changes. I don't have a lot of time on hand but this week-end would be a good time for me to solve a few pending issues. cheers :-) |
| # client loads metadata at init | ||
| client = KafkaClient(host='broker_1', port=4567) | ||
| self.assertItemsEqual({ | ||
| TopicAndPartition('topic_1', 0): brokers[0], |
There was a problem hiding this comment.
should this be brokers[1] based on:
topics['topic_1'] = { 0: PartitionMetadata('topic_1', 0, 1, [1, 2], [1, 2]) }
?
There was a problem hiding this comment.
Woah, great catch! incoming fix... I am using the wrong assertion which lead me to believe the test was right. The downsides of unit tests.
Conflicts: test/test_unit.py
Fix py26 compatibility issue, add mock to tox
|
I like this change. I also believe it should be merged soon because it looks like there are many pull requests that attempt to address the same problem, less effectively. |
|
Sorry for the wait, been super busy. I'll take another look at it soon if no one else does |
TopicAndPartition fix when partition has no leader = -1
Includes unittests and added mock dependency for unit tests