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

Use Celery Task event methods instead of updating at context manager level #3984

Closed
agjohnson opened this issue Apr 20, 2018 · 0 comments · Fixed by #8815
Closed

Use Celery Task event methods instead of updating at context manager level #3984

agjohnson opened this issue Apr 20, 2018 · 0 comments · Fixed by #8815
Assignees
Labels
Accepted Accepted issue on our roadmap Needed: design decision A core team decision is required
Milestone

Comments

@agjohnson
Copy link
Contributor

agjohnson commented Apr 20, 2018

Raising this because of #3864 and some issues we hit today.

Currently we're using context managers to try to grab and report all the exceptions that we can. We could actually be using Task.on_failure[0], Task.on_success[1], and Task.on_retry[2] though. Instead of grabbing and reporting exceptions to users through the context manager, instead just bubble these up to Celery and update the build state at task failure/sucess.

This would be tried in steps:

  • First, break out the update build code from the context manager. This will live in a utility function so it can be shared. Input is build_pk and the build state
  • Next, make an exception subclass that can bubble up to Celery. That is, one that is not reported to users, but raises up through the context managers
  • Make VCS locking a bubble up exception subclass, use autoretry_on to retry the task. (Do we mark as build failed here? Possible, but maybe we get away from that pattern)
  • Get rid of generic handling of errors outside buildenvs and use on_failure for this

At this point, we should be catching, reporting, and notifiying (we're not, see #3864) through the use of celery.

Next stage, if this is going smoothly, is to reduce the reporting logic on the context managers and see if celery can report these more consistently.

Overall, this will reduce the duplication in logic that we're using to report/update builds, and exceptions will be reported in one spot.

0: http://docs.celeryproject.org/en/latest/userguide/tasks.html#on_failure
1: http://docs.celeryproject.org/en/latest/userguide/tasks.html#on_success
2: http://docs.celeryproject.org/en/latest/userguide/tasks.html#on_retry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Needed: design decision A core team decision is required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants