Add GoodJob.preserve_job_records = :on_unhandled_error option to only preserve jobs that errored
#145
Conversation
|
I also have a bit of refactoring ready on https://github.com/tiramizoo/good_job/compare/preserve-on-error...tiramizoo:refactoring?expand=1 |
|
@morgoth Thank you for tackling this! I'm really excited to add the functionality. I'd like to retain (not deprecate) the Boolean values for preserve_job_records, and only add the one |
morgoth
force-pushed
the
preserve-on-error
branch
from
96ac50a
to
c988da4
Compare
|
ok, I removed the second commit |
There was a problem hiding this comment.
@morgoth I have some feedback, but I think we're close.
BTW, after writing out my code suggestion in this review, I went back to your refactoring branch and my suggestion looks nearly identical to your branch :-)
| @@ -209,10 +209,12 @@ def perform | |||
|
|
|||
| if rescued_error && GoodJob.reperform_jobs_on_standard_error | |||
| save! | |||
| elsif rescued_error && GoodJob.preserve_job_records == :on_error | |||
| update!(finished_at: Time.current) | |||
There was a problem hiding this comment.
I think this needs to be testing against self.error(any error that occurred during the job), rather than just rescued_error (errors that bubble up to the GoodJob backend).... or I think we should rename it specifically to be "on_backend_error"/"on_unhandled_error" (naming is hard).
Assuming that self.error matches with your needs for this, I think the entire block might be clearer with further rewriting (apologies it was messy in the first place). Something like:
if rescued_error && GoodJob.reperform_jobs_on_standard_error
save!
elsif GoodJob.preserve_job_records == true || (error && GoodJob.preserve_job_records == :on_error)
self.finished_at = Time.current
save!
else
destroy!
endThere was a problem hiding this comment.
I actually wanted saving only on "rescued" aka "unhandled" error, so I renamed the config to be :on_unhandled_error, as otherwise it would create a bit of noise to have multiple records of the same job/error.
It would be not clear if those entries are coming from the same AJ job or different ones.
This is matching the behavior of other AJ queue adapters.
I think storing on every error could also be an option (maybe another config value?) with some nice web UI grouping, so it's clear that given group of job exception is coming from the same AJ job.
There was a problem hiding this comment.
That is great. That makes me think (as a separate PR) that we should rename the configuration GoodJob. reperform_jobs_on_standard_error as GoodJob.reperform_jobs_on_unhandled_error. That configuration name was always unwieldy.
There was a problem hiding this comment.
@bensheldon Can I prepare a PR for renaming reperform_jobs_on_standard_error to reperform_jobs_on_unhandled_error with deprecation warning on old usage?
There was a problem hiding this comment.
@morgoth that would be great!
Question: what are your thoughts about an even briefer retry_on_unhandled_error?
|
@morgoth re:refactoring. you drew my attention to the unfortunate fact that the local variable |
| @@ -209,10 +209,12 @@ def perform | |||
|
|
|||
| if rescued_error && GoodJob.reperform_jobs_on_standard_error | |||
| save! | |||
| elsif rescued_error && GoodJob.preserve_job_records == :on_error | |||
| update!(finished_at: Time.current) | |||
There was a problem hiding this comment.
I actually wanted saving only on "rescued" aka "unhandled" error, so I renamed the config to be :on_unhandled_error, as otherwise it would create a bit of noise to have multiple records of the same job/error.
It would be not clear if those entries are coming from the same AJ job or different ones.
This is matching the behavior of other AJ queue adapters.
I think storing on every error could also be an option (maybe another config value?) with some nice web UI grouping, so it's clear that given group of job exception is coming from the same AJ job.
| retry_or_discard_error = GoodJob::CurrentExecution.error_on_retry || | ||
| GoodJob::CurrentExecution.error_on_discard | ||
| result_error = nil | ||
| result, result_error = nil, result if result.is_a?(Exception) # rubocop:disable Style/ParallelAssignment |
There was a problem hiding this comment.
Style: I think the stylecop should be preserved and this done as 2 lines.
| save! | ||
| elsif GoodJob.preserve_job_records == true || (GoodJob.preserve_job_records == :on_unhandled_error && unhandled_error) | ||
| update!(finished_at: Time.current) |
There was a problem hiding this comment.
Style: If a model object has been previously dirtied, save should be used rather than update.
bensheldon
changed the title
GoodJob.preserve_job_records = :on_unhandled_error option to only preserve jobs that errored
Adds option :on_error to preserve jobs in database only on final error and delete otherwise.
This is done in 2 commits. First one just introduces the functionality, the second one changes options to :never, :always, :on_error with backward compatibility.
closes #136