joowani / kq

Kafka-based Job Queue for Python
http://kq.readthedocs.io
MIT License
571 stars 24 forks source link

Dont commit on shutdown if task hasnt finished #5

Closed shashankmehra closed 7 years ago

shashankmehra commented 7 years ago

The issue I faced was that on shutdown of the worker if the worker was processing a task, it would abandon the task in the middle of its processing, and commit the record. This would result in half processed tasks which are never picked up again. Since this is a job queue, I have to make my tasks idempotent regardless, it makes sense to have another worker pick up the task instead of the task getting dropped. I have mitigated this issue for myself using these changes. Not sure if this is the best way to go about it. Please let me know your thoughts. It might also be a good idea to implement a graceful shutdown.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.6%) to 98.413% when pulling 48298536143ce5712bc4eb34c966873a20e01d61 on shashankmehra:missing_tasks into 5f89dd3142709f69eeca73ca7620c3ef8574c2a4 on joowani:dev.

joowani commented 7 years ago

Hi @shashankmehra,

I appreciate you bringing this to my attention. In the lastest release, I tweaked things so that consumers commit each time they process a record. This means we can actually remove the commiting completely from __del__. I went ahead and pushed the change to dev.

I also added a new function Queue.enqueue_with_key(key, func, *args, **kwargs) for convenience. Please test these out and let me know how they fare.

Because I rebased and modified the history, you will have run git reset --hard origin/dev.

Thanks again for your contribution. Joohwan

shashankmehra commented 7 years ago

Thanks. Removing the commit from __del__ makes things easier. enqueue_with_key also makes things easier since I am not forced to create a Job object on my own. I will be able to test it a little later and let you know.

shashankmehra commented 7 years ago

I was able to test enqueue_with_key and it works out for me. Thanks.

joowani commented 7 years ago

That's great to hear! Closing this out.