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

Support Revoked State #63

Merged
merged 4 commits into from
Oct 27, 2020
Merged

Support Revoked State #63

merged 4 commits into from
Oct 27, 2020

Conversation

EJH2
Copy link
Collaborator

@EJH2 EJH2 commented Oct 27, 2020

This PR adds (more) support for the REVOKED task state as discussed in #54. Notable changes in this PR include:

  • Added task_revoked_handler in websockets/tasks.py to handle revoked/terminated/expired tasks.
  • Progress.get_info() now uses celery.states for state comparisons. This should make the comparisons a tad clearer on what they actually are.
  • "Ready" tasks no longer use self.result.ready(), as that was intercepting REVOKED task states and not allowing us to pretty up the output before shipping to the frontend.

Sample outputs can be seen below:
image
image
image

@EJH2 EJH2 requested review from czue and OmarWKH October 27, 2020 02:23
Copy link
Owner

@czue czue left a comment

Choose a reason for hiding this comment

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

looks good to me, thanks!

"""Runs if a task has been revoked. This will be used to push a websocket update for revoked events.

If the websockets version of this package is not installed, this will fail silently."""
_result = ('terminated' if kwargs.pop('terminated') else None) or ('expired' if kwargs.pop('expired') else None) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the value used in line 79 in get_info? How is it populated for HTTP? I just want to ensure WS and HTTP behave the same because this looks hand-crafted but the HTTP code uses a single source (result.info).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the result.info value that gets used at that line, yes. HTTP populates this value by requesting the backend, which has the same values. The handler for revoked tasks does not pass the result, so I had to manually re-create the intended result in order to maintain parity with HTTP. All of the same data is there, it's just that rather than making a trip to the backend, I've opted to use what I've already been provided to do largely the same thing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. Unfortunate that result.info is not well documented in celery.

@EJH2
Copy link
Collaborator Author

EJH2 commented Oct 27, 2020

On second thought, I might remove the celery.states dependence, as the overlapping states in the elif may be doing more harm than good.

@EJH2 EJH2 merged commit 7de1298 into master Oct 27, 2020
@EJH2 EJH2 deleted the feat-support-revokes branch October 27, 2020 22:12
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.

3 participants