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

Set, serialize, and deserialize Active Job scheduled_at as Time; deserialize enqueued_at as Time; deprecate setting scheduled_at= with numeric/epoch #48066

Merged
merged 1 commit into from Sep 27, 2023

Conversation

bensheldon
Copy link
Contributor

@bensheldon bensheldon commented Apr 26, 2023

Motivation / Background

This Pull Request has been created to make a job's scheduled_at value available during job execution by serializing/deserializing the value.

This PR also aligns the behavior of both scheduled_at and enqueued_at to use Time objects internally instead of epoch-second Floats or Strings. This change for scheduled_at should be transparent for users that are using the primary set(wait:, wait_until:) interface and is non-breaking if setting the value directly e.g. job.scheduled_at = 10.minutes.from_now.to_f. The adapter interface is unchanged: adapters still receive epoch-second Float params in enqueue_at.

Detail

This change was initially proposed by @adamnoto in #32071 (2018), and closed because scheduled_at was viewed as an implementation detail of the backend queue adapter.

I am reopening this PR because I believe there have been some changes in the ecosystem, and with Active Job, that warrant revisiting how scheduled_at is handled.

Ecosystem changes: I'll point to my own good_job backend, also skiplock, that are designed for Active Job, and did not exist at the time the previous PR was closed five years ago. With these Active Job-native adapters, scheduled_at is directly used for scheduled jobs, and rescheduled retries. I think Active Job is in a stronger position today to assert the value, and if not appropriate, adapters are able to override/re-assign the value before execution, for example, as they typically do job.provider_job_id

Active Job changes: There is a greater interface focus on the instance of ActiveJob::Base. For example, introducing #set and the interface for perform_all_later. It would be nice to have full symmetry of attributes set on an instance before enqueue to be available on the instance after enqueue during execution.

The inspiration for reopening this change was that I recently saw the sidekiq-expiring-job gem and thought that it would be nice if that could be easily implemented in an Active Job before_perform. e.g.

class ApplicationJob < ActiveJob::Base
  ExpiredJobError = Class.new(StandardError)

  discard_on ExpiredJobError
  
  before_perform do |job|
    raise ExpiredJobError if job.scheduled_at < 30.minutes.ago
  end
end

Lastly, scheduled_at is not my favorite because it's a unix timestamp. If this is accepted, I'd love the ok to go through a deprecation cycle (or go through a deprecation cycle before accepting this PR) to convert it to a DateTime like enqueued_at is handled.

Additional information

Related issues:

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the activejob label Apr 26, 2023
@zzak zzak added this to the 7.1.0 milestone Sep 23, 2023
@zzak
Copy link
Member

zzak commented Sep 23, 2023

@bensheldon Could you rebase the changelog please? 🙏

@bensheldon
Copy link
Contributor Author

@zzak will do!

Do you have any thoughts about my last note on changing scheduled_at to be a DateTime instead of a Unix timestamp? On second thought, it should probably be changed before it's exposed in the job 😬

@bensheldon bensheldon changed the title Serialize and deserialize Active Job scheduled_at Set, serialize, and deserialize Active Job scheduled_at as Time; deserialize enqueued_at as Time Sep 24, 2023
@bensheldon bensheldon changed the title Set, serialize, and deserialize Active Job scheduled_at as Time; deserialize enqueued_at as Time Set, serialize, and deserialize Active Job scheduled_at as Time; deserialize enqueued_at as Time Sep 24, 2023
@bensheldon bensheldon force-pushed the aj_scheduled_at branch 3 times, most recently from b8745bc to c819a72 Compare September 24, 2023 16:28
@bensheldon
Copy link
Contributor Author

@zzak I've rebased and tests are green.

fyi, I did slightly expand the scope of this PR to internally treat scheduled_at as a Time object instead of epoch timestamp/numeric (it should preserve existing behavior if someone is setting/reading job.scheduled_at directly instead of going through set(wait:, wait_until:)). I also tweaked the deserialization of enqueued_at to deserialize it back from an ISO8601 String to a Time object so that both scheduled_at and enqueued_at are treated similarly.

@zzak
Copy link
Member

zzak commented Sep 25, 2023

@bensheldon I'm suggesting this change because it's scope was limited in size and improves the API to be succinct with the ecosystem, but won't be backportable once 7.1 is released. My assumption here is that this also doesn't break any compatibility or require a deprecation cycle, so that makes it easier to review. By making this suggestion, however, I'm only asking someone from core (like RF) to double check those assumptions. If you have any other stuff that fits that criteria, I think now is the time.

@bensheldon bensheldon force-pushed the aj_scheduled_at branch 5 times, most recently from 226bd4a to bb58050 Compare September 26, 2023 23:42
@bensheldon bensheldon changed the title Set, serialize, and deserialize Active Job scheduled_at as Time; deserialize enqueued_at as Time Set, serialize, and deserialize Active Job scheduled_at as Time; deserialize enqueued_at as Time; deprecate setting scheduled_at= with numeric/epoc Sep 26, 2023
@bensheldon bensheldon changed the title Set, serialize, and deserialize Active Job scheduled_at as Time; deserialize enqueued_at as Time; deprecate setting scheduled_at= with numeric/epoc Set, serialize, and deserialize Active Job scheduled_at as Time; deserialize enqueued_at as Time; deprecate setting scheduled_at= with numeric/epoc Sep 26, 2023
@bensheldon bensheldon changed the title Set, serialize, and deserialize Active Job scheduled_at as Time; deserialize enqueued_at as Time; deprecate setting scheduled_at= with numeric/epoc Set, serialize, and deserialize Active Job scheduled_at as Time; deserialize enqueued_at as Time; deprecate setting scheduled_at= with numeric/epoch Sep 26, 2023
@bensheldon bensheldon force-pushed the aj_scheduled_at branch 2 times, most recently from 5e9e50f to d81d549 Compare September 27, 2023 00:00
…serialize `enqueued_at` as Time; deprecate setting `scheduled_at=` with numeric/epoch

Co-authored-by: Adam Pahlevi <adam.pahlevi@gmail.com>
@rafaelfranca rafaelfranca merged commit bdbc182 into rails:main Sep 27, 2023
4 checks passed
@bensheldon
Copy link
Contributor Author

Noting that there was a deprecation warning generated by the test helper:

...and fixed here:

@bensheldon bensheldon deleted the aj_scheduled_at branch September 27, 2023 16:23
bronislav added a commit to bronislav/yabeda-activejob that referenced this pull request Oct 31, 2023
- Remove outdated versions of Rails (5.2, 6.0) and Ruby (2.5, 2.6) from test matrix due to reaching end of life
- Add new versions of Rails (7.1) and Ruby (3.2) to the test matrix
- Use separate gemfiles to enforce correct Rails version
- Parse `event.payload[:job].enqueued_at` only when needed. In Rails 7.1 it is an instance of `Time` which makes Time.parse fail (see rails/rails#48066)

Co-Authored-By: Jacopo Beschi <beschi.jacopo@gmail.com>
bronislav added a commit to bronislav/yabeda-activejob that referenced this pull request Nov 1, 2023
- Remove outdated versions of Rails (5.2, 6.0) and Ruby (2.5, 2.6) from test matrix due to reaching end of life
- Add new versions of Rails (7.1) and Ruby (3.2) to the test matrix
- Use separate gemfiles to enforce correct Rails version
- Parse `event.payload[:job].enqueued_at` only when needed. In Rails 7.1 it is an instance of `Time` which makes Time.parse fail (see rails/rails#48066)

Co-Authored-By: Jacopo Beschi <beschi.jacopo@gmail.com>
etsenake pushed a commit to Fullscript/yabeda-activejob that referenced this pull request Nov 1, 2023
- Remove outdated versions of Rails (5.2, 6.0) and Ruby (2.5, 2.6) from test matrix due to reaching end of life
- Add new versions of Rails (7.1) and Ruby (3.2) to the test matrix
- Use separate gemfiles to enforce correct Rails version
- Parse `event.payload[:job].enqueued_at` only when needed. In Rails 7.1 it is an instance of `Time` which makes Time.parse fail (see rails/rails#48066)

Co-authored-by: Jacopo Beschi <beschi.jacopo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants