-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Assistants: Don't affect longevity of tasks #1772
Conversation
|
||
self.setTime(600) | ||
self.setTime(self.sch._config.remove_delay + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't access private variables like _config. This breaks on the RPC tests.
34db7db
to
c7b9a54
Compare
When assistants where introduced, we imagined that one worker would run tasks (the assistant) and one would upload them. In order to ensure that the tasks don't get pruned before tasks get completed, we added a what we now consider an "antifeature" that made tasks not getting pruned while assistants where alive. Real world use have showed that it wasn't a good feature. It have caused bugs like spotify/luigi@736c0f1, but worst, it's a feature that makes luigid change behavior for all tasks if you just submit 1 assistant. I didn't expect that the first time I tested assistants. There's an email thread where it was discussed to remove this feature. In there one can also read about how to get something similar to the old behavior. https://groups.google.com/forum/#!topic/luigi-user/b7Acym0n-g4
c7b9a54
to
4fc3861
Compare
I found 1 bug already which I've amended a fix for now. It was that RUNNING tasks got pruned while the assistant was running them. sidenote: Looking through the code for the |
if (not task.stakeholders) and (task.remove is None) and (task.status != RUNNING): | ||
# We don't check for the RUNNING case, because that is already handled | ||
# by the fail_dead_worker_task function. | ||
logger.debug("Task %r has no stakeholders anymore -> might remove " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. The changed log message is not related to this patch. Just see it as a bonus update of legacy code.
Merging. I want to keep refactor other parts of the code without conflicts. :) |
* spotify/master: (24 commits) Add DateSecondParameter to parameter.py (spotify#1779) tox: Specify sphinx dependency better flake8: Unbreak travis build Excludes .tox from flake8 to prevent checking third-party libraries (spotify#1785) README: Remove monthly downloads badge Rename CentralPlannerScheduler to Scheduler (spotify#1781) Remove abstract Scheduler class (spotify#1778) Assistants: Don't affect longevity of tasks (spotify#1772) tests: Skip a inttermittently failing s3 test (spotify#1777) Update retcodes to handle new cases (spotify#1771) tests: Fix warning in remote_scheduler_test.py (spotify#1774) Remove sitecustomize file (spotify#1755) Fix exist method for ftp server Update copy() to return number and size of files copied Remove the confusing "dummy_test_module" directory (spotify#1756) Disable codecov comments on GitHub PRs (spotify#1754) Fix "owner_email" log message. (spotify#1762) docs: Install sphinx 1.4.4 in setup.py (spotify#1761) docs: Set minimum versions for sphinx (spotify#1760) Normalize ListParameter to be Immutable (spotify#1759) ...
Motivation and Context
When assistants where introduced, we imagined that one worker would
run tasks (the assistant) and one would upload them. In order to ensure
that the tasks don't get pruned before tasks get completed, we added
a what we now consider an "antifeature" that made tasks not getting
pruned while assistants where alive.
Real world use have showed that it wasn't a good feature. It have
caused bugs like 736c0f1, but worst, it's a feature that
makes luigid change behavior for all tasks if you just submit 1
assistant. I didn't expect that the first time I tested assistants.
There's an email thread where it was discussed to remove this feature.
In there one can also read about how to get something similar to the old
behavior.
https://groups.google.com/forum/#!topic/luigi-user/b7Acym0n-g4
Have you tested this? If so, how?
In addition to the modified tests, I'll try to test this in production for a few days. I'll report back.