Skip to content

Change MentionJob to MentionWorker and move to sidekiq #5312

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jan 2, 2020
Merged

Change MentionJob to MentionWorker and move to sidekiq #5312

merged 4 commits into from
Jan 2, 2020

Conversation

serenaf
Copy link
Contributor

@serenaf serenaf commented Dec 31, 2019 •
edited
Loading

What type of PR is this? (check all applicable)

  • Refactor

Description

I followed the PR here:
This creates a new MentionWorker and starts sending jobs to it from the Notification model. Once this is deployed, I will open a second PR to remove the old code.

Starting small, if this looks good, i can tackle more of the Notification jobs :)

Related Tickets & Documents

#5305

Added to documentation?

  • no documentation needed

@pr-triage pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Dec 31, 2019
@claassistantio
Copy link

claassistantio commented Dec 31, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

serenaf
serenaf commented Dec 31, 2019
View reviewed changes
app/workers/notifications/mention_worker.rb
@@ -0,0 +1,11 @@
module Notifications
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a notifications module to mimick the notifications module in the job directory

@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Dec 31, 2019
serenaf
serenaf commented Dec 31, 2019
View reviewed changes
app/workers/notifications/mention_worker.rb
class MentionWorker
include Sidekiq::Worker
sidekiq_options queue: :high_priority, retry: 10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not super sure about the queue here🙈 I presume sending Mentions to users should not be delayed heavily, but happy to change this :)

serenaf
serenaf commented Dec 31, 2019
View reviewed changes
app/workers/notifications/mention_worker.rb

def perform(mention_id)
mention = Mention.find_by(id: mention_id)
Notifications::NewMention::Send.call(mention) if mention
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to be passed in the old job code as an object. I am using it here directly, let me know you would like a further refactor

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine in this instance, because what you call is a class method, so there's no risk of mocking the wrong instance!

Copy link
Contributor

Choose a reason for hiding this comment

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

@rhymes separate from this PR, we have lots of jobs that do send if object which means when we dont have one we silently fail. What do you think about that? Do you think we should change that pattern to raise an error possibly?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @lightalloy has a better point of view on this but in general IIRC the gist is that we don't need to be alerted for resources that are not present in the DB 99% of the time. The other 1% we need to figure out case by case.

I think we shouldn't take this decision while we're doing this "moving" sprint. Let's move over the jobs to Sidekiq and then we can go through the jobs and decide which ones are worth bubbling the errors for.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rhymes is right about the motivation of adding the check. The job doesn't actually fail as it used to do before adding this checks, but gets canceled.

@pr-triage pr-triage bot added PR: draft bot applied label for PR's that are a work in progress and removed PR: unreviewed bot applied label for PR's with no review labels Dec 31, 2019
@serenaf serenaf marked this pull request as ready for review December 31, 2019 15:15
@serenaf serenaf requested a review from a team December 31, 2019 15:15
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Dec 31, 2019
@serenaf serenaf mentioned this pull request Dec 31, 2019
Closed
64 tasks
rhymes
rhymes reviewed Dec 31, 2019
View reviewed changes
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

This is a great PR @serenaf!

I think you're almost to the finish line, I left a note about the tests :)

app/workers/notifications/mention_worker.rb

def perform(mention_id)
mention = Mention.find_by(id: mention_id)
Notifications::NewMention::Send.call(mention) if mention
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine in this instance, because what you call is a class method, so there's no risk of mocking the wrong instance!

spec/workers/notifications/mention_worker_spec.rb Outdated
end

it "calls a service" do
perform_enqueued_jobs do
Copy link
Contributor

Choose a reason for hiding this comment

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

this is related to ActiveJob and can be removed

spec/workers/notifications/mention_worker_spec.rb Outdated
end

it "does nothing for non-existent mention " do
perform_enqueued_jobs do
Copy link
Contributor

Choose a reason for hiding this comment

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

this is related to ActiveJob and can be removed

mstruve
mstruve reviewed Dec 31, 2019
View reviewed changes
Copy link
Contributor

@mstruve mstruve left a comment

Choose a reason for hiding this comment

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

Thank you @serenaf for the PR!!! This looks great! Let's change that queue and delete some of those leftover ActiveJob specs and we will be ready to go!

app/workers/notifications/mention_worker.rb

def perform(mention_id)
mention = Mention.find_by(id: mention_id)
Notifications::NewMention::Send.call(mention) if mention
Copy link
Contributor

Choose a reason for hiding this comment

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

@rhymes separate from this PR, we have lots of jobs that do send if object which means when we dont have one we silently fail. What do you think about that? Do you think we should change that pattern to raise an error possibly?

spec/requests/notifications_spec.rb
perform_enqueued_jobs do
Mention.create_all(comment)
Notification.send_mention_notification(Mention.first)
Sidekiq::Testing.inline! do
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, I want to make a nice little testing helper for this but thank you for doing it here by hand!!!

@serenaf
Copy link
Contributor Author

serenaf commented Jan 1, 2020 •
edited
Loading

@mstruve @rhymes thanks so much for the great comments 🎉 I updated the PR to reflect the changes you wanted. And happy new year to all of you :) 🎉

mstruve
mstruve approved these changes Jan 2, 2020
View reviewed changes
Copy link
Contributor

@mstruve mstruve left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 2, 2020
@mstruve mstruve merged commit 51df325 into forem:master Jan 2, 2020
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 2, 2020
This was referenced Jan 3, 2020
Merged
Merged
@pr-triage pr-triage bot removed the PR: merged bot applied label for PR's that are merged label Jan 9, 2020
@pr-triage pr-triage bot added the PR: reviewed-approved bot applied label for PR's where reviewer approves changes label Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@lightalloy lightalloy lightalloy left review comments

@rhymes rhymes rhymes left review comments

@mstruve mstruve mstruve approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

PR: reviewed-approved bot applied label for PR's where reviewer approves changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@serenaf @claassistantio @lightalloy @rhymes @mstruve