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

Let worker wait for tasks running on other workers to complete. #225

Merged
merged 2 commits into from
Nov 15, 2013
Merged

Let worker wait for tasks running on other workers to complete. #225

merged 2 commits into from
Nov 15, 2013

Conversation

jimjh
Copy link
Contributor

@jimjh jimjh commented Nov 14, 2013

See #222 and #223.

Break up worker#run into smaller methods and give each a meaningful
name so it's easier to figure out what it's doing and allow code reuse.

Collapse duplicate if constructs.
@erikbern
Copy link
Contributor

Nice! However I think it should be configurable and default should be the existing behavior. As I just pointed out in issue #222, there might be some side effects like creating hundreds of worker processes just sitting around waiting for more work. So I'd rather make this an "experimental" feature that you can turn on if needed. Does that make sense?

@jimjh
Copy link
Contributor Author

jimjh commented Nov 14, 2013

I see where you are coming from. I guess I am using luigi slightly differently, and was operating under the assumption that all submitted tasks get run eventually unless there is a failure of some sort.

There is a real risk that the corner case you described could occur, especially if workflows are scheduled every minute. I will add a configuration option named worker-keep-alive and re-push.

If

- running tasks is non-empty, then another worker is running my tasks
- n_pending_tasks is non-zero, then I have something to do after she is done

And I shall wait for `wait_interval` seconds (with random jitter).

This adds the following configuration options:

- worker-keep-alive [boolean]
- worker-wait-interval [int]
@jimjh
Copy link
Contributor Author

jimjh commented Nov 14, 2013

OH YEA travis passed. I was wondering what the errors were about.

@erikbern
Copy link
Contributor

LGTM. Happy to merge unless anyone objects. I'll let it sit here for a day or two to give other people a chance

@jimjh
Copy link
Contributor Author

jimjh commented Nov 15, 2013

Merge?

erikbern pushed a commit that referenced this pull request Nov 15, 2013
Let worker wait for tasks running on other workers to complete.
@erikbern erikbern merged commit dc08c11 into spotify:master Nov 15, 2013
@jimjh jimjh deleted the worker-wait branch November 15, 2013 19:06
@duckontheweb
Copy link
Contributor

Did this ever make it's way into the documentation? I see that it's referenced in a couple of places, but I didn't see an entry in the [core] section of the configuration docs for it.

@dlstadther
Copy link
Collaborator

@duckontheweb it's in the [worker] section

@duckontheweb
Copy link
Contributor

It seems like the worker code here is getting that config from the [core] section, though. Am I misinterpreting that?

@dlstadther
Copy link
Collaborator

The legacy method was to provide the config_path=dict(section='core', name='worker-keep-alive'). So for legacy purposes, it will look under [core] for worker-keep-alive. But config classes (class worker(luigi.config)) will look under [worker] (the name of the class) for the parameter names listed. So in this case keep_alive.

So to be verbose in this example, you could provide

[core]
worker-keep-alive: True

or

[worker]
keep_alive: True

@duckontheweb
Copy link
Contributor

Perfect, thanks for clarifying that for me!

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.

None yet

4 participants