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

[RF-22752] Fix argument serializing when enqueuing failed job #43

Merged
merged 6 commits into from
Oct 27, 2021

Conversation

magni-
Copy link
Contributor

@magni- magni- commented Oct 27, 2021

When retrying a job, we call failed_job_class.restart_in, where failed_job_class is a QueueClassicPlus::Base descendant, and therefore serializes its arguments. #42 fixed an issue where we weren't deserializing arguments before sending them to restart_in, which meant we'd try to serialize already serialized arguments.

However, when enqueuing a failed job, we do not go through failed_job_class (since we're not enqueuing in the class's standard queue), and therefore do not re-serialize args. Instead we're calling JSON.dump on the deserialized arguments, which leads to things like "#<SomeClass:0x00007fbf70bc2b68>" getting enqueued. The easy fix is to use the raw (still-serialized) arguments when enqueuing a failed job.

@marvin-rfbot
Copy link

marvin-rfbot bot commented Oct 27, 2021

@magni-
Copy link
Contributor Author

magni- commented Oct 27, 2021

/reviewme @rainforestapp/devs @grodowski @pyromaniackeca @jbarber

Copy link
Contributor

@jbarber jbarber left a comment

Choose a reason for hiding this comment

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

Looks good.


## Releasing

Releasing is done in CircleCI via the `push_to_rubygems`, triggered by pushing a tagged commit. To do so, simply [create a new GitHub release](https://github.com/rainforestapp/queue_classic_plus/releases/new).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Releasing is done in CircleCI via the `push_to_rubygems`, triggered by pushing a tagged commit. To do so, simply [create a new GitHub release](https://github.com/rainforestapp/queue_classic_plus/releases/new).
Releasing is done in CircleCI via the `push_to_rubygems`, triggered by pushing a tagged commit. To do so, simply [create a new GitHub release](https://github.com/rainforestapp/queue_classic_plus/releases/new) or run `rake release:source_control_push`

(untested)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then you lose getting the changelog for free! (and you also need to have the Rubygems credentials locally)

Copy link
Contributor

Choose a reason for hiding this comment

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

and you also need to have the Rubygems credentials locally

this is incorrect for release:source_control_push, as it just manipulates the SCM:
https://github.com/ruby/ruby/blob/5af3f7f3574c16ec76fb44b21beec17a74f4417a/lib/bundler/gem_helper.rb#L75-L77

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😮 TIL

@magni- magni- merged commit 17377a6 into master Oct 27, 2021
@marvin-rfbot marvin-rfbot bot deleted the RF-22752/pp/serialize-failed-job branch October 27, 2021 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants