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

consider depreciating infinite timeout Consumer.Poll() #142

Closed
mhowlett opened this issue Apr 10, 2017 · 6 comments
Closed

consider depreciating infinite timeout Consumer.Poll() #142

mhowlett opened this issue Apr 10, 2017 · 6 comments

Comments

@mhowlett
Copy link
Contributor

Are there any practical situations where anyone would want to use Consumer.Poll() over a variant that has a timeout parameter?

Generally, you will want to poll the consumer with a relatively short timeout to allow controlled shutdown of the application.

Perhaps we want to add a variant that accepts a CancellationToken instead? Perhaps in that variant we would also want to encapsulate the poll loop?

@treziac
Copy link
Contributor

treziac commented Apr 17, 2017

Agree with this - the user can still call Poll(-1) to get the same (just add it in documentation), we don't want a new user to call Poll() by mistake and have a blocking thread for any reason

I liked the RdKafka Start and Stop in EventConsumer (basically just like the automatic poll loop in the Producer) Add a StartBackgroundPoll and StopBackgroudPoll has some sense

@tsibelman
Copy link

Hi do you planing to suppor async api for consumer ? Current implementation with Poll and event looks not very straightforward.

@treziac
Copy link
Contributor

treziac commented Jun 25, 2017

Hi!
Can you elaborate where an async method would make sense (compared to event or polling)? Could add it if there are proper usages.

Poll or consume won't do any action to the broker (internal fetch from broker are automatically done internally by the library by batch, poll/consume will just fetch from an internal queue, which is immediate)

An async method would be the same than calling Consume with a timeout (eventually 0) at the place where you except your await

Perhaps we could separate the consumer queue (which deliver consumer messages) from standard queue (metadata, commit callbak&other) by config to allow more scenario
Also we could not depreciate consume - I can see cases where we might not want event

(I'm not a confluent guy so they may have other opinion )

@tsibelman
Copy link

Looks like I did not fully understood the underlying system, earlier I was under impression that poll is a blocking IO operation, so I was thinking it would be more straight forward to consume bulks of messages from the network via async polling operations. If I use manual commit it would be best to commit it after I processed entire bulk.

@BenjaminHolland
Copy link

It seems like both Consume and Poll actually should be async in the case where the client-side event queue is empty, in which case it should return a task that returns when additional events have been enqueued.

Without consume, it's actually very difficult to do flow control properly, since I can't stop polling for messages without stopping the polling for other events. You could solve this by adding an event filter argument to Poll. That way you can keep the polling loop alive without missing or buffering messages.

@mhowlett
Copy link
Contributor Author

The 1.0 consumer has depreciated the Poll method, and introduced a Consume method with no timeout parameter but with CancellationToken instead. There is also a ConsumeAsync method, since there are scenarios that call for this, though that is not final (the implementation depends on Task.Run, which is not ideal).

The 'start' and 'stop' methods on the Consumer are definitely not coming back, though it may make sense to create a higher level class that builds on Consumer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants