Yelp / python-gearman

Gearman API - Client, worker, and admin client interfaces
http://github.com/Yelp/python-gearman/
Other
242 stars 123 forks source link

after_poll #49

Open debackel opened 11 years ago

debackel commented 11 years ago

Trying to test "after_poll" from a worker, I noticed that it is actually triggered 2 times at every poll cycle.

Looking at the worker.py code, “after_poll” is called from within “continue_while_connections_alive(any_activity)”, which is passed as the callback function when invoking “calling poll_connections_until_stopped”. Within “calling poll_connections_until_stopped” , the callback function is called before starting everything else, and within the loop.

May be I misunderstood the purpose of “after_poll”, but if the purpose is to have a method called every timeout, then the right place for calling “after_poll” would be, in my opinion, within the “if time_remaining == 0.0:” section.

But simply changing the place where the “callback_fxn” (“after_poll” in the case of a call from a worker) will not do. Indeed, the callback function is used not only to trigger the after_poll method in case poll_connections_until_stopped is called from a worker, but also to call a “continue_while_jobs_pending” method in case it is called from a client. So, we can't simply change the location of calls to “callback_fxn” within the “calling poll_connections_until_stopped” method.

I can see 2 solutions:

1/ Split the poll_connections_until_stopped method: one to be called by clients, one to be called by workers. In the worker version of it, do not re-enter the loop when timing out. This would mean that “work” should be included in an infinite loop (as it is done in the php library). The “after_poll” would simply not exist: additional code in the infinite loop would deal with it.

2/ Add a “after_timeout(any_activity)” method in the worker and client classes, which would be called from within poll_connections_until_stopped method.

The second solution is simpler to implement, in my opinion. But we would loose the possibility to stop the polling from within after_poll (which is not an issue, in my opinion).

Does it make sense? Am I missing something? Should I propose a patch ? This is at the very heart of the code, so I'm afraid to miss a use case.

debackel commented 11 years ago

Up. Could anyone comment on this topic?

klange commented 11 years ago

We are in the midst of a rather extensive rewrite of some of the core internals for both workers and clients which should address this, as well as a number of other issues.

debackel commented 11 years ago

Thank you sir. Just for my organisation (to decide whether to wait a bit for the new version or to continue with the existing), do you have a rough estimate of the release date for this new version?

klange commented 11 years ago

We expect at least a preliminary beta in the next few weeks.

svisser commented 9 years ago

@klange is there more information on the rewrite and/or new version? I'm contemplating whether I should invest more time in adding unit tests and Python 3.x compatibility for https://github.com/Yelp/python-gearman/pull/63 or whether it's better to wait.