Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Consumer falsiness. #342

Closed
wants to merge 1 commit into from
Closed

Conversation

wting
Copy link
Contributor

@wting wting commented Mar 9, 2015

I ran into an issue where 0 (a legitimate partition id) is interpreted as the same as None and thus giving me the results for all partitions (e.g. in Consumer.pending()).

@dpkp
Copy link
Owner

dpkp commented Mar 9, 2015

partitions is a list... [] is false, [0] is true. can you provide a test case? I dont think this PR does what you think it does.

@wting
Copy link
Contributor Author

wting commented Mar 9, 2015

If I call Consumer.pending(0) I'm (incorrectly) expecting the list of pending messages for partition id 0. However this will fail the falsy check if not partitions: and returns pending messages for all partitions.

It is preferable to fail fast and complain that the function expects a list and not an integer.

[15] λ consumer.fetch_offsets.items()
[15] » [(0, 0)]

[16] λ consumer.pending(0)
[16] » 101

[17] λ consumer.pending([0])
[17] » 101

[18] λ consumer.pending(1)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-18-a9f596a67196> in <module>()
----> 1 consumer.pending(1)

/nail/home/wting/corner/pfpg/virtualenv_run/lib/python2.6/site-packages/kafka/consumer/base.pyc in pending(self, partitions)
    161         reqs = []
    162 
--> 163         for partition in partitions:
    164             reqs.append(OffsetRequest(self.topic, partition, -1, 1))
    165 

TypeError: 'int' object is not iterable

[19] λ consumer.pending([1])
---------------------------------------------------------------------------
UnknownTopicOrPartitionError              Traceback (most recent call last)
<ipython-input-19-e533b14c0e3a> in <module>()
----> 1 consumer.pending([1])

/nail/home/wting/corner/pfpg/virtualenv_run/lib/python2.6/site-packages/kafka/consumer/base.pyc in pending(self, partitions)
    164             reqs.append(OffsetRequest(self.topic, partition, -1, 1))
    165 
--> 166         resps = self.client.send_offset_request(reqs)
    167         for resp in resps:
    168             partition = resp.partition

/nail/home/wting/corner/pfpg/virtualenv_run/lib/python2.6/site-packages/kafka/client.pyc in send_offset_request(self, payloads, fail_on_error, callback)
    455             payloads,
    456             KafkaProtocol.encode_offset_request,
--> 457             KafkaProtocol.decode_offset_response)
    458 
    459         out = []

/nail/home/wting/corner/pfpg/virtualenv_run/lib/python2.6/site-packages/kafka/client.pyc in _send_broker_aware_request(self, payloads, encoder_fn, decoder_fn)
    156         for payload in payloads:
    157             leader = self._get_leader_for_partition(payload.topic,
--> 158                                                     payload.partition)
    159 
    160             payloads_by_broker[leader].append(payload)

/nail/home/wting/corner/pfpg/virtualenv_run/lib/python2.6/site-packages/kafka/client.pyc in _get_leader_for_partition(self, topic, partition)
     87         # If the partition doesn't actually exist, raise
     88         if partition not in self.topic_partitions[topic]:
---> 89             raise UnknownTopicOrPartitionError(key)
     90 
     91         # If there's no leader for the partition, raise

UnknownTopicOrPartitionError: TopicAndPartition(topic='topic', partition=1)

@dpkp
Copy link
Owner

dpkp commented May 18, 2015

Ok -- I see your point and think this is worth merging, though I am going to add a separate issue to support the interface you were expecting: Consumer.pending(0)

dpkp pushed a commit that referenced this pull request May 18, 2015
@dpkp
Copy link
Owner

dpkp commented May 18, 2015

merged

@dpkp dpkp closed this May 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants