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

Projects
None yet
2 participants
@agjohnson
Contributor

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.

Give TaskStep class knowledge of the underlying task
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

This comment has been minimized.

Member

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)
@agjohnson

This comment has been minimized.

Contributor

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

@agjohnson agjohnson merged commit 0f90a1a into master Apr 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@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