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

[bug-fix] scheduler won't override running status incorrectly #2187

Merged
merged 2 commits into from
Aug 8, 2017

Conversation

stguido
Copy link
Contributor

@stguido stguido commented Jul 18, 2017

Description

The scheduler can currently override the RUNNING status of a task with something incorrect coming from a different worker other than the one currently running the task.

Motivation and Context

The issue has been discussed in the mailing list here: https://groups.google.com/forum/#!searchin/luigi-user/stefano|sort:relevance/luigi-user/WCiQfNEfWY4/5zGdX5X6BgAJ

Have you tested this? If so, how?

It passes tests. We are about to push this into our production env as well.

Copy link
Contributor

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

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

Sounds good, but change important as this would require a test case too I think. See the scheduler api tests.

@stguido
Copy link
Contributor Author

stguido commented Jul 25, 2017

@Tarrasch I've added a test case for the particular fix. I'm not sure how to parse the build failure, it seems a git/build issue? Should I create a new PR?

@Tarrasch
Copy link
Contributor

@stguido, can you rebase your code on master? This problem is fixed now but you need to rebase your code for your patch to "include" it. :)

@stguido stguido force-pushed the scheduler-status-bug-upstream branch from deb4b1e to 307aa53 Compare July 26, 2017 18:10
@stguido stguido force-pushed the scheduler-status-bug-upstream branch from 307aa53 to 98875bb Compare August 7, 2017 21:49
@stguido
Copy link
Contributor Author

stguido commented Aug 8, 2017

@Tarrasch, I've finally got this to be green again - lmk if it's good for a merge

@Tarrasch Tarrasch merged commit daf9ce9 into spotify:master Aug 8, 2017
@Tarrasch
Copy link
Contributor

Tarrasch commented Aug 8, 2017

Thanks for the fix @stguido! :)

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.

2 participants