-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Change UpdateReactableJob to UpdateReactableWorker and move to sidekiq #5551
Change UpdateReactableJob to UpdateReactableWorker and move to sidekiq #5551
Conversation
As a high frequency job I think this should be done in two steps: add the worker and then remove the old job in a separate PR |
Sure, I will do this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @luchiago, thanks for this PR as well, there a few things to fix but we're almost there!
class UpdateReactableWorker | ||
include Sidekiq::Worker | ||
|
||
sidekiq_options queue: :low_priority, retry: 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would raise the priority, what do you think @mstruve ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, both of the actions in this worker are simple very fast SQL updates. I'm wondering if we save ourselves the complexity and do them inline without creating a whole job for it. @rhymes thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we even need this worker since the commands it is executing are 2 very fast SQL commands. Regardless, if we do decide to keep it we do need to change the without_delay
method
class UpdateReactableWorker | ||
include Sidekiq::Worker | ||
|
||
sidekiq_options queue: :low_priority, retry: 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, both of the actions in this worker are simple very fast SQL updates. I'm wondering if we save ourselves the complexity and do them inline without creating a whole job for it. @rhymes thoughts?
@mstruve I think it's warranted, the job now does this:
which calls
I'd say it's okay to keep it because of point 1. We can measure its performance in DD I guess and remove it in the future |
@rhymes So, should I raise the priority #5551 discuss? And what about the spec #5551 discuss? It's related to |
@rhymes could you look at my last comment
|
@luchiago sorry for the delay :(( yes please raise the priority and investigate why that spec is failing for you, thank you! ps. there's also a conflict up here, I guess it's because in the meantime other PRs have been merged :( |
Sure @rhymes. I will send the changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @luchiago!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
@@ -80,7 +80,7 @@ def touch_user | |||
end | |||
|
|||
def update_reactable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called in an after_save
callback which means the reaction will not be persisted to the database yet. We need to move this to an after_commit
callback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @luchiago for all your help with this project!!! 🤗
@luchiago I hopped in here real quick and fixed the failing spec. That was a result of some code that I had put into master so I figured I'd help out. Thank you so much for taking such a strong initiative on moving these jobs over to workers, it has been a H-U-G-E help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks @luchiago and @atsmith813!
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents
#5305
Added to documentation?