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

Let Sidekiq and Que set provider_job_id #20056

Merged
merged 2 commits into from May 7, 2015

Conversation

jvanbaarsen
Copy link
Contributor

@jvanbaarsen jvanbaarsen commented May 7, 2015

When a job is added to Sidekiq or Que by ActiveJob, make sure we still can get the
original job_id provider by Sidekiq or Que. We do this by adding the Sidekiq jid or Que job_id to
provider_job_id field on the job object.

Partly fixes #18821

This PR is an extension on the PR submitted by @kddeisz : #19910

@mperham
Copy link
Contributor

mperham commented May 7, 2015

Thank you! This is a pretty critical integration point with Sidekiq since all its APIs require the JID to work.

@jvanbaarsen jvanbaarsen changed the title Let Sidekiq set provider_job_id Let Sidekiq and Que set provider_job_id May 7, 2015
@jvanbaarsen
Copy link
Contributor Author

jvanbaarsen commented May 7, 2015

Added Que job_id reporting as well.

@kddnewton
Copy link
Contributor

kddnewton commented May 7, 2015

Nice work @jvanbaarsen

@rafaelfranca
Copy link
Member

rafaelfranca commented May 7, 2015

@jvanbaarsen could you rebase your branch? Thanks

@jvanbaarsen
Copy link
Contributor Author

jvanbaarsen commented May 7, 2015

@rafaelfranca Yes! will do

When a job is added to Sidekiq by ActiveJob, make sure we still can get the
original job_id provider by Sidekiq. We do this by adding the sidekiq jid to
provider_job_id field on the job object.

Partly fixes rails#18821

Signed-off-by: Jeroen van Baarsen <jeroenvanbaarsen@gmail.com>
@kddnewton
Copy link
Contributor

kddnewton commented May 7, 2015

You might also want to make your adapter tests use the same test as the
delayed job one now - and just check for not null. We don't need both tests.

On Thu, May 7, 2015 at 3:48 PM, Jeroen van Baarsen <notifications@github.com

wrote:

@rafaelfranca https://github.com/rafaelfranca Yes! will do


Reply to this email directly or view it on GitHub
#20056 (comment).

Kevin D. Deisz
TrialNetworks - part of DrugDev
Software Developer
383 Elliot Street, Suite G
Newton, MA 02464
+1 617.952.4071 x134 (office)
+1 703.615.0396 (mobile)
kdeisz@trialnetworks.com

@jvanbaarsen
Copy link
Contributor Author

jvanbaarsen commented May 7, 2015

@kddeisz Ah yeah, good point! Will do as well

@jvanbaarsen
Copy link
Contributor Author

jvanbaarsen commented May 7, 2015

@rafaelfranca You want me to squash the commits as well?

@rafaelfranca
Copy link
Member

rafaelfranca commented May 7, 2015

It is fine these two commits

@@ -1,3 +1,8 @@
* Allow `Sidekiq` and `que` to report the job id back to `ActiveJob::Base` as
Copy link
Member

@rafaelfranca rafaelfranca May 7, 2015

Choose a reason for hiding this comment

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

Instead of adding a new entry maybe we should change the entry bellow. WDYT?

Copy link
Contributor Author

@jvanbaarsen jvanbaarsen May 7, 2015

Choose a reason for hiding this comment

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

@rafaelfranca Yes sounds good.

Copy link
Contributor

@kddnewton kddnewton May 7, 2015

Choose a reason for hiding this comment

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

Seems fine

Copy link
Contributor

@kddnewton kddnewton May 7, 2015

Choose a reason for hiding this comment

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

While we're here, are there any other adapters that need this? resque?

On Thu, May 7, 2015 at 3:58 PM, Jeroen van Baarsen <notifications@github.com

wrote:

In activejob/CHANGELOG.md
#20056 (comment):

@@ -1,3 +1,8 @@
+* Allow Sidekiq and que to report the job id back to ActiveJob::Base as

@rafaelfranca https://github.com/rafaelfranca Yes sounds good.


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/20056/files#r29889786.

Kevin D. Deisz
TrialNetworks - part of DrugDev
Software Developer
383 Elliot Street, Suite G
Newton, MA 02464
+1 617.952.4071 x134 (office)
+1 703.615.0396 (mobile)
kdeisz@trialnetworks.com

Signed-off-by: Jeroen van Baarsen <jeroenvanbaarsen@gmail.com>

Fixes #18821.

*Kevin Deisz*
*Kevin Deisz* And *Jeroen van Baarsen*
Copy link
Contributor Author

@jvanbaarsen jvanbaarsen May 7, 2015

Choose a reason for hiding this comment

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

@kddeisz you agree with this?

Copy link
Contributor

@kddnewton kddnewton May 7, 2015

Choose a reason for hiding this comment

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

I'm fine with that!

On Thu, May 7, 2015 at 4:00 PM, Jeroen van Baarsen <notifications@github.com

wrote:

In activejob/CHANGELOG.md
#20056 (comment):

 Fixes #18821.
  • Kevin Deisz
  • Kevin Deisz And Jeroen van Baarsen

@kddeisz https://github.com/kddeisz you agree with this?


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/20056/files#r29889974.

Kevin D. Deisz
TrialNetworks - part of DrugDev
Software Developer
383 Elliot Street, Suite G
Newton, MA 02464
+1 617.952.4071 x134 (office)
+1 703.615.0396 (mobile)
kdeisz@trialnetworks.com

@jvanbaarsen
Copy link
Contributor Author

jvanbaarsen commented May 7, 2015

@rafaelfranca I think its ready to be merged (Once the tests are green 💃 )

rafaelfranca added a commit that referenced this pull request May 7, 2015
Let Sidekiq and Que set provider_job_id
@rafaelfranca rafaelfranca merged commit d19d79e into rails:master May 7, 2015
@jvanbaarsen jvanbaarsen deleted the sidekiq-job-id branch May 7, 2015
@mperham
Copy link
Contributor

mperham commented May 7, 2015

Will this get into 4.2 or 5+ only?

@rafaelfranca
Copy link
Member

rafaelfranca commented May 7, 2015

5+ only.

@diddeb
Copy link

diddeb commented May 20, 2015

@mperham So, in current versions there's no way to find the underlying jid from Sidekiq using ActiveJob?

@mperham
Copy link
Contributor

mperham commented May 20, 2015

Not that I'm aware of.

On May 20, 2015, at 05:58, Didde Brockman notifications@github.com wrote:

@mperham So, in current versions there's no way to find the underlying jid from Sidekiq using ActiveJob?


Reply to this email directly or view it on GitHub.

@jvanbaarsen
Copy link
Contributor Author

jvanbaarsen commented May 20, 2015

@diddeb Not out of the box. Only way you can achieve this, is by monkeypatching.

@diddeb
Copy link

diddeb commented May 20, 2015

Thanks @jvanbaarsen + @mperham... Freedom Patching it is.

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.

ActiveJob not returning true job_id
6 participants