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

Per Task Retry-Policy #1791

Merged
merged 9 commits into from
Aug 2, 2016
Merged

Conversation

javrasya
Copy link
Contributor

Description

Defining retry-policy per task as it is defined in #1073

Motivation and Context

It may be required to have different retry-policy for different tasks according to their priority and resource usage. Assume that, a task is retrieving data from Hive which responses in long time and other one is retrieving data from RDBM which responses in short time. I may want Hive task not to retry many times and not to drain all my resource again an again. But, retrying RDBM task many times may not be problem for me. Assume that, I have some network issue for a while and I should be able to give its retry-count more than Hive task. So I would need to define retry-policy at task level.

This fixes the issue defined in #1073

Have you tested this? If so, how?

  • Some unit tests are added about the feature.
  • Run available tests(Not all of them, I don't know, some of them maybe includes hadoop ones, I couldn't make passed. ) via tox and travis.
  • PyCharm Debugging.

What is added?

  • The feature
  • Unit tests
  • Documentation
  • An example

… this patch, luigi is able to supports defining retry-policy per task. disable-num-failures config name is depreceated and it is retry_count now.
| disable_hard_timeout | scheduler |
+------------------------+-----------+
| disable_window_seconds | scheduler |
+------------------------+-----------+
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually we shouldn't have this information here as it's already somewhere else in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should include a part about what is in retry policy and what they can use per task,what do you think about it?

@Tarrasch
Copy link
Contributor

Tarrasch commented Jul 29, 2016

Can you also add a test case with with dynamic dependencies? It should be super easy, and it immediately shows if it's necessary to thread along the deps_retry_policy_dicts var or not.

edit: Oh, I see that you actually have no tests that sets up a task, sets the configuration and then checks that it behaves as expected. You should and this for sure. You basically just need to copy paste your included example but make it into a testcase. And while at it make one version with run()time dependencies.

…mple. PerTaskRetryPolicyBehaviorTest has been added in worker_test. unittest.main() part is removed.
@javrasya
Copy link
Contributor Author

@Tarrasch, I added multiple test cases in PerTaskRetryPolicyBehaviorTest in worker_test. You can see deps_retry_policy is important by removing the part below and run PerTaskRetryPolicyBehaviorTest again.

scheduler.py -> add_task()

retry_policy=self._generate_retry_policy(deps_retry_policy_dicts.get(dep))

This is because of luigi task adding order. Luigi adds a dependency task indirectly with its parent task first. So we need to send deps_retry_policy of its parent task unless luigi adds dependency task directly instead of indirectly with its parent task.

def setUp(self):
self.per_task_retry_count = 2
self.default_retry_count = 1
self.sch = Scheduler(retry_delay=0.1, retry_count=self.default_retry_count, prune_on_get_work=True, record_task_history=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to be so explicit and say record_task_history=False. But it's ok. :)

@Tarrasch
Copy link
Contributor

Tarrasch commented Aug 1, 2016

@javrasya, I still think deps_retry_policy can be removed. To make it clearer how, I created javrasya#1 :)

Implementation without deps_retry_policy_dicts
…class is now extended from LuigiTestCase. DbTaskHistoryTest is fixed after changes including adding retry_policy which is not optional parameter for scheduler Task.
@Tarrasch Tarrasch merged commit bfb6233 into spotify:master Aug 2, 2016
@Tarrasch
Copy link
Contributor

Tarrasch commented Aug 2, 2016

And this is now finally in!! :)

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