Add Reschedule, Discard, Retry Job buttons to Dashboard #425
Conversation
bensheldon
force-pushed
the
retry_jobs
branch
from
81f279b
to
16ca0db
Compare
| @@ -123,5 +133,30 @@ def running? | |||
| advisory_locked? | |||
| end | |||
| end | |||
|
|
|||
| def retry_job | |||
There was a problem hiding this comment.
I'm wondering if it wouldn't be easier to just change the schedule_at timestamp to current time, so it is handled in the regular way instead of doing it inline.
Wouldn't that work for some reason?
| active_job = execution.active_job | ||
|
|
||
| raise AdapterNotGoodJobError unless active_job.class.queue_adapter.is_a? GoodJob::Adapter | ||
| raise ActionForStateMismatchError unless status == :discarded |
There was a problem hiding this comment.
I think it would be useful to run the job even if it is not discarded yet. It could be called "Execute now" on UI or similar.
There was a problem hiding this comment.
Agreed. I'm imagining there being Reschedule Now, and also Discard (e.g. cancel it if it's already scheduled).
My intention is to try to perform the actions in the most ActiveJob manner (e.g. to have callbacks working), which requires some complexity. It may end up being more complexity than I can stomach though. I'm trying to keep the language symmetrical with ActiveJob (e.g. Retry, Discard, Re-Schedule).
There was a problem hiding this comment.
@morgoth I updated the PR to include 3 actions. I'd love your feedback.
I think potentially there is a 4th action too which is "create a new job with the same parameters" ("Duplicate job"?)
bensheldon
force-pushed
the
retry_jobs
branch
from
16ca0db
to
b1da1a1
Compare
bensheldon
force-pushed
the
retry_jobs
branch
from
b1da1a1
to
3acc600
Compare
bensheldon
force-pushed
the
retry_jobs
branch
from
3acc600
to
18fcc1c
Compare
bensheldon
changed the title
bensheldon
force-pushed
the
retry_jobs
branch
from
18fcc1c
to
3c27b52
Compare
| end | ||
| end | ||
|
|
||
| def reschedule_job(scheduled_at = Time.current) |
There was a problem hiding this comment.
Why do we need reschedule and retry? I find it a bit confusing on the UI. I guess having single "Schedule now" or "Execute now" would be simpler.
Also what about https://github.com/bensheldon/good_job/pull/425/files#r730604390
Wouldn't work to update scheduled_at and remove finished_at for that case to have it run in the regular way?
There was a problem hiding this comment.
I think I misunderstood your first comments. And it isn't helped by me not having added docs to any of this. I'll try to answer the conceptual stuff here, and then add the code documentation.
- I would like to match the verbs of ActiveJob (e.g.
retry,discard), which I'm hoping helps with cognitive load (both as a GoodJob maintainer and end-developer/operator) - I admit there is messy conceptual overlap in the implementation between
JobandExecution. A Job has many Executions, but the Job itself is not modeled in the database (In the DB, the Job is a group-by of Executions). I'd like to keep Executions immutable as much as possible. e.g. if the execution has errored, running the job again creates a new execution, rather than un-executing the previous execution. My perspective may be overly weighted towards the ideas expressed in Better Defaults and Run-time expectations #271. My adherence to this idea is what makes it complicated.
But I get the point that, from the developer/operator perspective, you want a button that is "do it!".
Your leading-edge feedback is super valuable. I was imagining this going a few different ways:
- It being too fiddly a set of controls (which I think is where you're coming from).
- Liking the fiddly controls, but being hampered by the state-based filtering making them difficult to use efficiently (e.g. people are reading the state of a job by seeing which controls are available)
- Finding this inefficient and wanting mass-actions of "do all these jobs!"
- Getting some left-field stuff like "We have a compliance regime that means we can't have any operator discretion. Please make optional"
- Not getting any feedback at all
There was a problem hiding this comment.
Here's the next steps I'm thinking:
- Put documentation into this PR to explain the class/methods and why it's complicated
- Release this with the fiddly buttons
- Make a GH Discussion topic for getting freeform "Dashboard Improvement Suggestions" to link to in the release notes, and I'll see if I can pin it.
- At this point, it's fairly simple to have a "do it!" method that calls the underlying implementations based on state, so we can quickly adjust to that too.
There was a problem hiding this comment.
Sure, that's a good plan. Better to have something on which we all can iterate and provide/receive feedback
| <% end %> | ||
|
|
||
| <% job_discardable = job.status.in? [:scheduled, :retried, :queued] %> | ||
| <%= button_to discard_job_path(job.id), method: :put, class: "btn btn-sm #{job_discardable ? 'btn-outline-primary' : 'btn-outline-secondary'}", form_class: "d-inline-block", disabled: !job_discardable, aria: { label: "Discard job" }, title: "Discard job" do %> |
There was a problem hiding this comment.
I think this might be handy but also a bit confusing on how it differs from "delete" (BTW are you planning to have a delete here?)
Maybe having a title "Stop job from executing but leave in the database" would be more clear?
bensheldon
changed the title
|
Schedule Now should also be an option on cron jobs |
bensheldon
force-pushed
the
retry_jobs
branch
from
3c27b52
to
86c4826
Compare
|
@ssendev I'll do those cron dashboard improvements in another PR 👍 |
Connects to #256.
The Retry action is only available upon Discarded Jobs. I'm considering having each job have all of the actions displayed but disabled if they are not relevant for the current state of the action. Once there are more actions, it should be clearer whether this is good or not.Adds Schedule Now, Retry, and Discard buttons to Jobs. All of the buttons are present but are disabled if not relevant to the current job state.