Add deleting jobs from UI. #265

morgoth · 2021-05-28T06:44:26Z

morgoth · 2021-05-28T06:45:10Z

engine/app/controllers/good_job/active_jobs_controller.rb

@@ -4,5 +4,10 @@ def show
      @jobs = GoodJob::Job.where("serialized_params ->> 'job_id' = ?", params[:id])
                          .order(Arel.sql("COALESCE(scheduled_at, created_at) DESC"))
    end
+
+    def destroy
+      GoodJob::Job.where("serialized_params ->> 'job_id' = ?", params[:id]).delete_all


I used the same lookup as for show page, but why are we using AJ job id here? Wouldn't be easier to just use db id?

When an ActiveJob is retried, it generates a new GoodJob resource. That GJ resource has a new primary ID, but the ActiveJob ID is the same. In other words, a single ActiveJob job's history is represented across multiple GoodJob job entries.

I think you have the implementation right here, by deleting all of the GoodJob resources that have the same ActiveJob ID.

...but it's not pretty. And when thinking about "retry" in the future, we have to make sure that it's operating on the latest/newest/head GoodJob resource for a particular ActiveJob job.

To put it another way, GoodJob::Job resources are "runs".

This also makes me wonder about the safety of having the "delete" on the GJ Job, rather than on the Show page itself to represent that it's deleting all the runs. But this also doesn't have to be perfect.

I think the worry I have is that someone will delete a job that has another run that is currently executing or waiting to execute.

Generative/blue sky idea: delete could be wrapped with an advisory lock on all the matching records.

Ah right. We have GoodJob.preserve_job_records = :on_unhandled_error so I never seen this.

So I think that for deleting we should use record id to delete only given record, as I believe this is what would be expected, by the principle of least surprise. WDYT?

I changed it, to remove only one, given job.
I believe to be more REST-ful we could change the show action to be actually the index one with applied filter.

Thanks, I think that's least surprise to only remove the one job 👍

I want to suggest moving the destroy action into good_jobs#destroy because it is acting upon a good_job record. It could be clearer by having a GoodJobJobsController and an ActiveJobJobsController maybe?

I want to be fairly permissive at this stage because I'm learning both the functionality that people want, and it's also helpful to think through these things with you on actual PRs 😄

That makes sense to not mix both, however what do you think about:

I believe to be more REST-ful we could change the show action to be actually the index one with applied filter.

This way all the actions fit into one controller.
Also currently the "show" action can display many jobs, which might be confusing.

In my mind, the active_jobs#show controller is still displaying a single ActiveJob Job, of which the details are multiple GoodJob Jobs.

I am imagining that this page would, in the future, also have some roll-up statistics (when the ActiveJob Job was initially enqueued, whether it has finished running, total number of retries, etc.).

As an end-user, the ActiveJob job is the thing I imagine most users are conceptualizing and the GoodJob Job is the a "run" or "attempt". Unfortunately there isn't a single database record that models the ActiveJob Job yet for the purpose of display.

Thanks for explanation. I moved it out to separate controller

refs bensheldon#256

morgoth · 2021-05-28T06:56:41Z

spec/system/dashboard_spec.rb

@@ -5,4 +5,17 @@
    visit '/good_job'
    expect(page).to have_content 'GoodJob 👍'
  end
+
+  # This requires storing job in database, but rest of the tests is using inline adapter


When I add this to config/environments/test.rb the unit tests are failing

config.active_job.queue_adapter = :good_job config.good_job = {execution_mode: :external}

I will look into that.

@morgoth You can add this:

ActiveJob::Base.queue_adapter = GoodJob::Adapter.new(execution_mode: :external)

The adapter is reset after every test, so that's should be all you need:

bensheldon

This is fantastic! Thank you.

It does uncover some conceptual issues of how GoodJob jobs related to ActiveJob jobs, but no time like the present to figure it out 😄

bensheldon · 2021-05-28T14:42:28Z

engine/app/controllers/good_job/active_jobs_controller.rb

@@ -4,5 +4,10 @@ def show
      @jobs = GoodJob::Job.where("serialized_params ->> 'job_id' = ?", params[:id])
                          .order(Arel.sql("COALESCE(scheduled_at, created_at) DESC"))
    end
+
+    def destroy
+      GoodJob::Job.where("serialized_params ->> 'job_id' = ?", params[:id]).delete_all


When an ActiveJob is retried, it generates a new GoodJob resource. That GJ resource has a new primary ID, but the ActiveJob ID is the same. In other words, a single ActiveJob job's history is represented across multiple GoodJob job entries.

I think you have the implementation right here, by deleting all of the GoodJob resources that have the same ActiveJob ID.

...but it's not pretty. And when thinking about "retry" in the future, we have to make sure that it's operating on the latest/newest/head GoodJob resource for a particular ActiveJob job.

To put it another way, GoodJob::Job resources are "runs".

bensheldon · 2021-05-28T14:52:09Z

engine/app/views/layouts/good_job/base.html.erb

+      <div class="alert alert-success alert-dismissible fade show d-flex align-items-center offset-md-3 col-6" role="alert">
+        <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" fill="currentColor" class="bi bi-check-circle-fill flex-shrink-0 me-2" viewBox="0 0 16 16">
+          <path d="M16 8A8 8 0 1 1 0 8a8 8 0 0 1 16 0zm-3.97-3.03a.75.75 0 0 0-1.08.022L7.477 9.417 5.384 7.323a.75.75 0 0 0-1.06 1.06L6.97 11.03a.75.75 0 0 0 1.079-.02l3.992-4.99a.75.75 0 0 0-.01-1.05z" />
+        </svg>


Can these SVGs be extracted into a shared partial?

do you mean separate partial for each SVG?

bensheldon · 2021-05-28T14:52:30Z

spec/system/dashboard_spec.rb

@@ -5,4 +5,17 @@
    visit '/good_job'
    expect(page).to have_content 'GoodJob 👍'
  end
+
+  # This requires storing job in database, but rest of the tests is using inline adapter


I will look into that.

bensheldon · 2021-05-28T14:55:40Z

engine/app/controllers/good_job/active_jobs_controller.rb

@@ -4,5 +4,10 @@ def show
      @jobs = GoodJob::Job.where("serialized_params ->> 'job_id' = ?", params[:id])
                          .order(Arel.sql("COALESCE(scheduled_at, created_at) DESC"))
    end
+
+    def destroy
+      GoodJob::Job.where("serialized_params ->> 'job_id' = ?", params[:id]).delete_all


This also makes me wonder about the safety of having the "delete" on the GJ Job, rather than on the Show page itself to represent that it's deleting all the runs. But this also doesn't have to be perfect.

I think the worry I have is that someone will delete a job that has another run that is currently executing or waiting to execute.

Generative/blue sky idea: delete could be wrapped with an advisory lock on all the matching records.

bensheldon

👍

bensheldon · 2021-06-04T14:14:13Z

I just released this as v1.9.6.

morgoth commented May 28, 2021

View reviewed changes

Add deleting jobs from UI.

6ed6bc7

refs bensheldon#256

morgoth force-pushed the delete-jobs branch from 593b70e to 6ed6bc7 Compare May 28, 2021 06:55

morgoth commented May 28, 2021

View reviewed changes

bensheldon reviewed May 28, 2021

View reviewed changes

morgoth force-pushed the delete-jobs branch from 23a6d19 to 76bf1b2 Compare June 1, 2021 05:58

Improve deleting jobs

8e64c8e

morgoth force-pushed the delete-jobs branch from 76bf1b2 to 8e64c8e Compare June 1, 2021 06:00

Move deleting jobs to own controller

443b464

bensheldon approved these changes Jun 1, 2021

View reviewed changes

bensheldon merged commit df35a44 into bensheldon:main Jun 4, 2021

bensheldon mentioned this pull request Jun 4, 2021

Allow job management (retry, destroy) through the Web UI #256

Closed

bensheldon added the enhancement New feature or request label Jun 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deleting jobs from UI. #265

Add deleting jobs from UI. #265

morgoth commented May 28, 2021 •

edited

Loading

morgoth May 28, 2021

bensheldon May 28, 2021

bensheldon May 28, 2021

morgoth May 28, 2021

morgoth Jun 1, 2021

bensheldon Jun 1, 2021

morgoth Jun 1, 2021

bensheldon Jun 1, 2021

morgoth Jun 1, 2021

morgoth May 28, 2021

bensheldon May 28, 2021

bensheldon May 31, 2021

bensheldon left a comment

bensheldon May 28, 2021

bensheldon May 28, 2021

morgoth May 28, 2021

bensheldon May 28, 2021

bensheldon May 28, 2021

bensheldon left a comment

bensheldon commented Jun 4, 2021

Add deleting jobs from UI. #265

Add deleting jobs from UI. #265

Conversation

morgoth commented May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bensheldon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bensheldon left a comment

Choose a reason for hiding this comment

bensheldon commented Jun 4, 2021

morgoth commented May 28, 2021 •

edited

Loading