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

Assistants: Don't affect longevity of tasks #1772

Merged
merged 1 commit into from
Jul 20, 2016

Conversation

Tarrasch
Copy link
Contributor

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.


self.setTime(600)
self.setTime(self.sch._config.remove_delay + 1)
Copy link
Contributor

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.

@Tarrasch Tarrasch force-pushed the simplify-assistants-pruning branch from 34db7db to c7b9a54 Compare July 19, 2016 03:24

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
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
@Tarrasch Tarrasch force-pushed the simplify-assistants-pruning branch from c7b9a54 to 4fc3861 Compare July 19, 2016 07:47
@Tarrasch
Copy link
Contributor Author

Tarrasch commented Jul 19, 2016

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 scheduler.py, I feel it can be improved in so many ways. Maybe I'll take a stab at it unless somebody else wants to. :)

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 "
Copy link
Contributor Author

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.

@Tarrasch Tarrasch merged commit 405f48d into spotify:master Jul 20, 2016
@Tarrasch Tarrasch deleted the simplify-assistants-pruning branch July 20, 2016 10:02
@Tarrasch
Copy link
Contributor Author

Merging. I want to keep refactor other parts of the code without conflicts. :)

p7k pushed a commit to Celmatix/luigi that referenced this pull request Jul 28, 2016

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
* 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)
  ...
This was referenced Jun 29, 2022
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

2 participants