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

use :poolboy.status to get free workers #67

Merged
merged 2 commits into from
May 14, 2016

Conversation

mitchellhenke
Copy link
Contributor

To fix #59

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 8e6e1aa on mitchellhenke:poolboy_full into * on edgurgel:master*.

@@ -193,6 +193,8 @@ defmodule Verk.WorkersManager do
Log.start(job, worker)
Verk.Worker.perform_async(worker, self, job)
notify!(%Events.JobStarted{job: job, started_at: Timex.DateTime.now})
:full ->
Verk.enqueue(job)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not that simple. We should never ask for jobs if we don't have a free worker. To push back a job we need to remove from the inprogress list as well. Dequeuing a job is done by this: https://github.com/edgurgel/verk/blob/master/priv/mrpop_lpush_src_dest.lua

So if we "need" to reenqueue a job I would rather consider this an invalid state that we got to and it's a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I remove this clause?

This should be unnecessary now that poolboy is used to determine how many available workers there are?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. We should always get the amount of workers we know that poolboy has. It should never be less than the amount we just asked for I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Dropped that part of the case clause.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling dc42370 on mitchellhenke:poolboy_full into * on edgurgel:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 4fafb41 on mitchellhenke:poolboy_full into * on edgurgel:master*.

@edgurgel
Copy link
Owner

Thanks! 🎉

@edgurgel edgurgel merged commit 3bf1704 into edgurgel:master May 14, 2016
@edgurgel
Copy link
Owner

Released as part of https://github.com/edgurgel/verk/releases/tag/v0.11.1

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

Successfully merging this pull request may close these issues.

:poolboy.checkout returning :full
3 participants