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

Serialized Params and ActiveJob extensions #362

Closed
giacomoguiulfo opened this issue Sep 4, 2021 · 3 comments · Fixed by #363
Closed

Serialized Params and ActiveJob extensions #362

giacomoguiulfo opened this issue Sep 4, 2021 · 3 comments · Fixed by #363

Comments

@giacomoguiulfo
Copy link

giacomoguiulfo commented Sep 4, 2021

Hi @bensheldon, thanks for your work with good_job, it's quite amazing 🙌

I'm reaching out with a few questions and would love to get your thoughts.

Most recently, I've been trying try to use an ActiveJob extension called job-iteration together with GoodJob. This gem essentially offers a way to make jobs that operate over a large dataset more resilient by essentially making them "iterable". It leverages Ruby's Enumerator class to "batch" the datasets/data structures, and keeps track of a "cursor/checkpoint" that represents how much of the "batch/iterations" has been processed. Then, when a worker is shutdown or after a time threshold is reached, the job reschedules itself (if it hasn't completed processing all data) but with an updated cursor as part of the serialized params, so that next time the job re-runs, it starts from where the cursor left off. The core logic for this can find here. The most important methods are perform, interruptible_perform, iterate_with_enumerator, serialize, and deserialize.

Unfortunately, this extension didn't work out of the box with good_job because it saves the "cursor" on the job's serialized_params, which for good_job in particular, it is currently set to readonly,: https://github.com/bensheldon/good_job/blob/main/lib/good_job/job.rb#L21. To my impression, it seems that change was made as a preventive measure, rather than to fix a bug?

In any case, I tried out removing that attr_readonly line and it all worked like a charm!

So I guess my question is, could be possible for that attribute to be writable again? How strong is the case for it being readonly and why?
If you don't think it's feasible to make it writable, is there a pattern in which to extend good_job when "state" is required, perhaps through a well-known extension jsonb column? Such that I can make changes on the extension side if that's the case.
Or any way in which we could make these 2 work? Or perhaps, there's a better solution to the problem this extension solves, that good_job is more suited for?

Thanks in advance! :)

@bensheldon
Copy link
Owner

@giacomoguiulfo thank you for opening this issue! TIL there are ActiveJob extensions; that's awesome! Let's get it working with GoodJob.

The reason for the attr_readonly :serialized_params is two reasons, general and specific:

  • A GoodJob design decision that each run/execution/retry of a job is individually tracked as a record
  • When ActiveJob retries after an error, it modifies the execution_exceptions hash that's part of serialized_params without duping it, which can dirty up the current run's record. That was the reason for Ensure Job#serialized_params are immutable #218

I want GoodJob to be compatible with this extension. I think there are two things to do:

  1. Have more targeted protection for the execution_exceptions data and remove the attr_readonly
  2. (probably) file an upstream bug with Rails to get ActiveJob to dup the data before modifying it

@bensheldon
Copy link
Owner

bensheldon commented Sep 6, 2021

@giacomoguiulfo I have the problem fixed in #363 and removed the attr_readonly. Please let me know if you run into any other problems and thank you again for opening the issue!

Released in v2.0.5

@giacomoguiulfo
Copy link
Author

Amazing! You're fast. It now works like a charm. Thanks a lot @bensheldon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants