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

Implement GoodJob.cleanup_preserved_jobs, fixes #351 #356

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ separate isolated execution pools with semicolons and threads with colons.

#### `good_job cleanup_preserved_jobs`

`good_job cleanup_preserved_jobs` deletes preserved job records. See [`GoodJob.preserve_job_records` for when this command is useful.
`good_job cleanup_preserved_jobs` deletes preserved job records. See `GoodJob.preserve_job_records` for when this command is useful.

```bash
$ bundle exec good_job help cleanup_preserved_jobs
Expand Down Expand Up @@ -724,7 +724,8 @@ It is also necessary to delete these preserved jobs from the database after a ce
- For example, in a Rake task:

```ruby
GoodJob::Job.finished(1.day.ago).delete_all
GoodJob.cleanup_preserved_jobs # Will keep 1 day of job records by default.
GoodJob.cleanup_preserved_jobs(before_seconds_ago: 7.days.seconds) # It also takes custom arguments.
aried3r marked this conversation as resolved.
Show resolved Hide resolved
```

- For example, using the `good_job` command-line utility:
Expand Down
23 changes: 22 additions & 1 deletion lib/good_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ module GoodJob
# By default, GoodJob deletes job records after the job is completed successfully.
# If you want to preserve jobs for latter inspection, set this to +true+.
# If you want to preserve only jobs that finished with error for latter inspection, set this to +:on_unhandled_error+.
# If +true+, you will need to clean out jobs using the +good_job cleanup_preserved_jobs+ CLI command.
# If +true+, you will need to clean out jobs using the +good_job cleanup_preserved_jobs+ CLI command or
# by using +Goodjob.cleanup_preserved_jobs+.
# @return [Boolean, nil]
mattr_accessor :preserve_job_records, default: false

Expand Down Expand Up @@ -114,6 +115,26 @@ def self._shutdown_all(executables, method_name = :shutdown, timeout: -1)
end
end

# Deletes preserved job records.
# By default, GoodJob deletes job records when the job is performed and this
# method is not necessary. However, when `GoodJob.preserve_job_records = true`,
# the jobs will be preserved in the database. This is useful when wanting to
# analyze or inspect job performance.
# If you are preserving job records this way, use this method regularly to
# delete old records and preserve space in your database.
# @params before_seconds_ago [nil,Numeric] Jobs olders than this will be deleted (default: +86400+).
# @return [Integer] Number of jobs that were deleted.
def self.cleanup_preserved_jobs(before_seconds_ago: nil)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm curious if you think it would be good to have a better kwarg name here. I want to suggest something like

  • cleanup_preserved_jobs(before: 3.days.ago) or
  • cleanup_preserved_jobs(older_than: 3.days) <-- I think I like this one.
  • Something better?

The longer option names were really a result of wanting the CLI interface to be clear given that there are limitations in what can be passed through ENV. But as a Ruby method, it's easy to pass in a timestamp, or duration.

It's a kwarg so it's trivial to preserve both options, and there is a benefit to being symetrical with the CLI options, but I recognize it's a little clunky.

Not a blocker of this PR, I wanted to get your perspective :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm curious if you think it would be good to have a better kwarg name here.

Definitely!

When writing this, I was also considering older_than but wanted to stick with the CLI option name to avoid having to discuss a potential other name. So I'm very happy you brought it up!

I don't think we necessarily need to keep both names, but I'm open to that. I'll work on the changes and you can have another look if you want to keep this symmetrical after all.

before_seconds_ago ||= GoodJob::Configuration.new({}).cleanup_preserved_jobs_before_seconds_ago
timestamp = Time.current - before_seconds_ago

ActiveSupport::Notifications.instrument("cleanup_preserved_jobs.good_job", { before_seconds_ago: before_seconds_ago, timestamp: timestamp }) do |payload|
deleted_records_count = GoodJob::Job.finished(timestamp).delete_all

payload[:deleted_records_count] = deleted_records_count
end
end

def self._executables
[].concat(
CronManager.instances,
Expand Down
11 changes: 1 addition & 10 deletions lib/good_job/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,7 @@ def cleanup_preserved_jobs

configuration = GoodJob::Configuration.new(options)

timestamp = Time.current - configuration.cleanup_preserved_jobs_before_seconds_ago

ActiveSupport::Notifications.instrument(
"cleanup_preserved_jobs.good_job",
{ before_seconds_ago: configuration.cleanup_preserved_jobs_before_seconds_ago, timestamp: timestamp }
) do |payload|
deleted_records_count = GoodJob::Job.finished(timestamp).delete_all

payload[:deleted_records_count] = deleted_records_count
end
GoodJob.cleanup_preserved_jobs(before_seconds_ago: configuration.cleanup_preserved_jobs_before_seconds_ago)
end

no_commands do
Expand Down
2 changes: 1 addition & 1 deletion lib/good_job/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class Configuration
DEFAULT_DEVELOPMENT_ASYNC_POLL_INTERVAL = -1
# Default number of threads to use per {Scheduler}
DEFAULT_MAX_CACHE = 10000
# Default number of seconds to preserve jobs for {CLI#cleanup_preserved_jobs}
# Default number of seconds to preserve jobs for {CLI#cleanup_preserved_jobs} and {GoodJob.cleanup_preserved_jobs}
DEFAULT_CLEANUP_PRESERVED_JOBS_BEFORE_SECONDS_AGO = 24 * 60 * 60
# Default to always wait for jobs to finish for {Adapter#shutdown}
DEFAULT_SHUTDOWN_TIMEOUT = -1
Expand Down
34 changes: 34 additions & 0 deletions spec/lib/good_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,38 @@
end.to change(described_class, :shutdown?).from(true).to(false)
end
end

describe '.cleanup_preserved_jobs' do
let!(:recent_job) { GoodJob::Job.create!(finished_at: 12.hours.ago) }
let!(:old_unfinished_job) { GoodJob::Job.create!(scheduled_at: 2.days.ago, finished_at: nil) }
let!(:old_finished_job) { GoodJob::Job.create!(finished_at: 36.hours.ago) }

it 'deletes finished jobs' do
deleted_jobs_count = described_class.cleanup_preserved_jobs

expect(deleted_jobs_count).to eq 1

expect { recent_job.reload }.not_to raise_error
expect { old_unfinished_job.reload }.not_to raise_error
expect { old_finished_job.reload }.to raise_error ActiveRecord::RecordNotFound
end

it 'takes arguments' do
deleted_jobs_count = described_class.cleanup_preserved_jobs(before_seconds_ago: 10)

expect(deleted_jobs_count).to eq 2

expect { recent_job.reload }.to raise_error ActiveRecord::RecordNotFound
expect { old_unfinished_job.reload }.not_to raise_error
expect { old_finished_job.reload }.to raise_error ActiveRecord::RecordNotFound
end

it 'is instrumented' do
allow(ActiveSupport::Notifications).to receive(:instrument).and_call_original

described_class.cleanup_preserved_jobs

expect(ActiveSupport::Notifications).to have_received(:instrument).at_least(:once)
end
end
end