Skip to content

Retry Failed Job #1325

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

ticosax
Copy link
Contributor

@ticosax ticosax commented Feb 12, 2025

This is an action intended to be manually invoked that put the failed job's state back to todo, while keeping the events history.

Only integrated within django (for now)
It's an Early submission (draft) for the purpose of getting feedback the maintainers. I would like to get confirmation that's something they'd like to consider, assuming I'll push it through with tests and updated documentation.
only Tested manually on my machine as it is.

Closes N/A

Successful PR Checklist:

  • Tests
    • (not applicable?)
  • Documentation
    • (not applicable?)

PR label(s):

@github-actions github-actions bot added the PR type: feature ⭐️ Contains new features label Feb 12, 2025
@ewjoachim
Copy link
Member

Hello!

I like the idea and large parts of the work already done.
I'm not sure I think a retried status is the best though. We had issues when we introduced a aborting state, which we had to undo. I think a job can be both retried AND todo, or doing, or suceeded or failed. If we want to capture that, it would rather be, I think, an event in the events table (capturing the fact a same job might be retried multiple times).

I like the idea of Django admin actions. but this immediately bears the idea of also aborting a task through the admin, though that might be for a later PR.

@ticosax
Copy link
Contributor Author

ticosax commented Mar 3, 2025

Hi, thanks for your feedback.
I'm not sure I completely understand your recommendations though :)

I'm not sure I think a retried status is the best though.

I agree I haven't introduced one.

If we want to capture that, it would rather be, I think, an event in the events table

Exactly, we are in agreement. and the PR does that already ...

Am I hallucinating or you reviewed it a bit too quickly :)

Nevertheless, I will add some tests it will make feature clearer on how it's supposed to work, and maybe it will reveal my hallucinations.
Stay tuned

@ticosax
Copy link
Contributor Author

ticosax commented Mar 3, 2025

In addition to django admin, do you see a need to expose this feature on the cli ?
Where else could it be useful ?

@ticosax ticosax force-pushed the retry-failed-job branch from 5f2464e to 123aa2d Compare March 3, 2025 15:36
@github-actions github-actions bot added the PR type: documentation 📚 Contains documentation updates label Mar 3, 2025
@ewjoachim
Copy link
Member

Hm, maybe I have, I'll take some time time and reread through, apologies.

@ticosax ticosax marked this pull request as ready for review March 20, 2025 14:29
@ticosax ticosax requested a review from a team as a code owner March 20, 2025 14:29
ticosax and others added 6 commits April 2, 2025 09:56
@ticosax ticosax force-pushed the retry-failed-job branch 2 times, most recently from 0bae760 to cace783 Compare April 2, 2025 08:18
Copy link

github-actions bot commented Apr 2, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  procrastinate
  manager.py 522-523
  testing.py
  worker.py
Project Total  

This report was generated by python-coverage-comment-action

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR type: documentation 📚 Contains documentation updates PR type: feature ⭐️ Contains new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants