Skip to content
This repository has been archived by the owner on Mar 24, 2021. It is now read-only.

Make delivery report threadlocal by default, but switchable #483

Closed
johnistan opened this issue Mar 7, 2016 · 4 comments
Closed

Make delivery report threadlocal by default, but switchable #483

johnistan opened this issue Mar 7, 2016 · 4 comments
Milestone

Comments

@johnistan
Copy link
Contributor

The explanation for why the delivery_report queue is thread local make sense:

#344 (comment)

However I have a use case where i would like a centralize handler of delivery exceptions. That is many "producers" and one background thread monitor the delivery_report queue. If you guys know of a way to accomplish this given the current implementation, please let know and this change might be unneeded.

What are thoughts on allowing the delivery_report to optionally be a global queue. (something like delivery_report_queue_thread_local defaults True.) There is an obvious tradeoff between api complexity and flexibility.

@emmettbutler
Copy link
Contributor

Hi @jofusa. I think this is a reasonable idea, but I'm wary of making the api more complicated than it truly needs to be. Whether or not this is a case that warrants the additional complexity isn't clear to me right now.

The solution I suggested earlier still looks like it could work. Your application can maintain a global queue. Then, for each thread you have calling produce(), you can have it call get_delivery_report() and put the result into your application's global queue. Your application can then spawn another thread that reads from this queue and performs error handling based on the delivery reports it emits.

If you tried this solution and found real problems with it that I'm not thinking of at the moment, it would make a strong case for making the internal queue switchable between local and global. If not, we could actually add code based on your example to the documentation as a standard way to do global error handling.

@yungchin
Copy link
Contributor

What if the API change amounts to this: in addition to the current True and False options, we allow delivery_reports to accept a custom class that implements _DeliveryReportQueue. So then we could define a class DeliveryGlobalQueue and maybe define a config object under pykafka.common, so that you could call

topic.get_producer(delivery_reports=pykafka.common.DeliveryReports.GLOBAL)

and the current True and False would of course still work. Would that keep it simple enough?

@emmettbutler emmettbutler modified the milestones: 2.3.1, 2.4.0 Mar 22, 2016
@johnistan
Copy link
Contributor Author

I like the callback mechanism discussed in #506 over a switchable queue. (as long as it also reported failures). I think i would work in my case better.

@emmettbutler
Copy link
Contributor

Closing in favor of #506

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

No branches or pull requests

3 participants