Yelp / zygote

A Python HTTP process management utility.
http://opensource.yelp.com/
Apache License 2.0
40 stars 15 forks source link

Pass io_loop instance as **kwargs for get_application #19

Closed smoy closed 12 years ago

smoy commented 12 years ago

This change is driven by google-safe-browsing's need to schedule fetches of additional blacklist data. Instead of using a separate thread, exposing zygote/worker.py's io_loop allow application to schedule work on that io_loop.

Roguelazer commented 12 years ago

This is an API-breaking change. Would it be better to implement this the way that initialize was implemented (e.g., check to see if the application exposed a "set_ioloop" function of some kind, and, if so, call it with the io_loop)? Or do we not care?

Eskil? Do you have thoughts?

smoy commented 12 years ago

I like that. Perserving backward compatibility and avoiding magic are nice. I will wait for eskil's comment before submitting another pull request.

eskil commented 12 years ago

Bear with me here, for I am not that familiar with the zygote code yet.

The existing code passes the command line options to get_application as _args. But the geocoder eg. only takes *_kwargs right now. So we already have problems.

I prefer smoy's approach of passing any extra info in a _kwargs, and use the args for the command line options. And then pass additional things in *_kwargs. Seems more pythonic than having the apps create a new "set_foo" function if we add arguments.

Yes, it's API breaking, but I think it's for the better in this case.

smoy commented 12 years ago

**kwargs seems to provide a generic mechanism to expose additional information from zygote. If zygote install base is still small, I suggest we take the opportunity to make an API breaking change rather now than later.

eskil commented 12 years ago

Smoy, I'll all good with this, but I think there's a merge conflict. The githubs won't let me merge this automatically.

smoy commented 12 years ago

I am going pull upstream changes and try to submit another pull request.

smoy commented 12 years ago

Close this request as I have submitted pull request #21 after merging with upstream changes.