-
Notifications
You must be signed in to change notification settings - Fork 210
feat(python): add AsyncIterator interface to IggyConsumer #2169
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
base: master
Are you sure you want to change the base?
Conversation
b98f4d8 to
830a592
Compare
|
@changhc I'd love to get your feedback w.r.t the interface. |
This adds a `.iter()` method for asynchronously iterating over Iggy messages from a consumer. Adding this method requires the consumer group to be initialized so `consumer_group` is made async and initialization is made upfront. `partition_id` is also exposed on `ReceiveMessage` to allow manual commit of offsets.
830a592 to
2ce2ba0
Compare
|
|
||
| /// Asynchronously iterate over `ReceiveMessage`s. | ||
| #[gen_stub(override_return_type(type_repr="collections.abc.AsyncIterator[ReceiveMessage]", imports=("collections.abc")))] | ||
| fn iter<'a>(&self) -> ReceiveMessageIterator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any problem making this __aiter__?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, I thought about it as well but landed on having a iter method because I thought it read better. I don't have strong feelings about this so am open to changing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this again and now I think it's better not to allow users to iterate through the consumer itself as that can be confusing. This iter method can probably be named as read_messages for clarity as it seems to be reading messages only instead of consuming them.
Also, what is the expected behaviour?
count = 0
async for message in consumer.iter():
# read messages 0 to 4
count += 1
if count == 5:
break
# call it again
async for message in consumer.iter():
# reads message 5 or 0?
break
If this method changes the consumer state, we need to make this clear.
|
Sorry for dropping the ball here. I've had reduced bandwidth and I apologize for not communicating that I've had to put this on hold. I'm going to need this for work in a bit so I will prioritize this soon. What's next for this PR is that we have to think about the interface we want to expose. Adding this as I'll try to get into this ASAP. |
|
@cstruct mate, no worries, no one is demanding anything from you :D i just wanted to cleanup PRs so i pinged you. there are no deadlines. if you don't have time that's perfectly fine - just write it, don't force anything upon yourself. |
This adds a
.iter()method for asynchronously iterating over Iggy messages from a consumer. Adding this method requires the consumer group to be initialized soconsumer_groupis made async and initialization is made upfront.partition_idis also exposed onReceiveMessageto allow manual commit of offsets.