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

Give TaskStep class knowledge of the underlying task #3983

Merged
merged 1 commit into from Apr 20, 2018

Conversation

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Apr 20, 2018

There is no self.retry on the task step class, this exists on the task.

This also fixes a bug where we weren't handling the lock on the python step as
well.

We discussed a better pattern to use here would be to rely on celery on_failure
and on_retry event methods more. This would simplify logic here greatly.

There is no `self.retry` on the task step class, this exists on the task.

This also fixes a bug where we weren't handling the lock on the python step as
well.

We discussed a better pattern to use here would be to rely on celery on_failure
and on_retry event methods more. This would simplify logic here greatly.
@agjohnson agjohnson requested a review from humitos Apr 20, 2018
@agjohnson agjohnson added the Bug label Apr 20, 2018
@agjohnson agjohnson requested a review from davidfischer Apr 20, 2018
@humitos
Copy link
Member

@humitos humitos commented Apr 20, 2018

We discussed a better pattern to use here would be to rely on celery on_failure
and on_retry event methods more. This would simplify logic here greatly.

After taking a look at these changes. I think what I want is something like:

  • keep the context manager do the work they are doing right now (handle BuildEnvironmentError, Warning, and thrown exception by us)
  • use the on_failure to catch all unhandled exceptions plus all the exceptions (known) that we are handling in the task itself (e.g. LockTimeout and others)

Loading

@agjohnson
Copy link
Contributor Author

@agjohnson agjohnson commented Apr 20, 2018

Yup, agreed. I think the first pass at this solves the email notification issue as well as some general code cleanup around exceptions

Loading

@agjohnson agjohnson merged commit 0f90a1a into master Apr 20, 2018
1 check passed
Loading
@agjohnson agjohnson deleted the agj/project-task-retry branch Apr 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants