Update Sidekiq and Sidekiq cron/unique job gems to latest versions #3217
Conversation
murny
requested review from
ConnorSheremeta,
jefferya,
lagoan and
pgwillia
as code owners
| @@ -462,6 +462,8 @@ GEM | |||
| rdoc (6.3.3) | |||
| redcarpet (3.6.0) | |||
| redis (4.8.1) | |||
| redis-client (0.17.0) | |||
There was a problem hiding this comment.
Sidekiq brought this in, which is now clashing with our own module RedisClient which will then create the following error:
As mentioned we can just use a different name. I used Jupiter::Redis instead.
| gem 'sidekiq-cron' | ||
| gem 'sidekiq-unique-jobs', '~> 7.1' | ||
| gem 'sidekiq', '~> 7.1' | ||
| gem 'sidekiq-cron', '~> 1.10' |
There was a problem hiding this comment.
Need to upgrade this one as well as with the new Sidekiq v7, some apis were changed:
I went through the changelogs of all 3 of these gems, nothing seem super out of the ordinary. Sidekiq itself had some breaking changes, nothing that we cared about in our application.
config/initializers/redis.rb
Outdated
| module Jupiter | ||
| module Redis | ||
| def self.current | ||
| @redis ||= ConnectionPool::Wrapper.new do |
There was a problem hiding this comment.
Wrapped this in a ConnectionPool. I assume this is fine? Not sure if there was a reason not to do this in the past, but it seems to work well locally.
I'm using ConnectionPool::Wrapper which will only use a single global connection as an in between of using a full on connection pool and the existing code: https://github.com/mperham/connection_pool#migrating-to-a-connection-pool
config/initializers/redis.rb
Outdated
| module Redis | ||
| def self.current | ||
| @redis ||= ConnectionPool::Wrapper.new do | ||
| ::Redis.new(url: Rails.application.secrets.redis_url) |
There was a problem hiding this comment.
Need the :: notation (shorthand for Object::) as when we refer to Redis here, it thinks the inner Jupiter::Redis. But we want the "global" redis, e.g: Object::Redis which is the redis gem code.
murny
force-pushed
the
upgrade-sidekiq-gems
branch
from
49cee5f
to
493c6ad
Compare
…do this since Jupiter::Redis already doing it
| $queue ||= ConnectionPool.new(size: 1, timeout: 5) { RedisClient.current } | ||
|
|
||
| $queue.with do |connection| | ||
| Jupiter::Redis.current.with do |connection| |
There was a problem hiding this comment.
@lagoan this is probably a stretch but... do you think initializing the connection pool here could have been a source for any of the errors we were tracking down with PMPY?
There was a problem hiding this comment.
Curious what the errors were? I mentioned here that the previous ConnectionPool with a size of 1, could have caused timeouts perhaps 🤔 #3217 (comment). So if you were seeing a ton of timeouts this might have been why?
There was a problem hiding this comment.
These were the errors: ualbertalib/pushmi_pullyu#311 (comment)
Yeah, Dependabot had an odd dependency resolution thing with flipper recently too #3171 (comment) I couldn't find an issue that matched in the dependabot-core repo on a quick search, though Grouped Updates sounds interesting.
Looks good. This will remove some redundant initialization of the ConnectionPool too! |
Context
This PR upgrades Sidekiq and sidekiq-cron/sidekiq-unique-job gems to the latest version.
Dependabot has been attempting to do this in the past, like this latest one here: #3214 (not sure why for some reason its descriptions are saying it's updating oaysis/rdf/etc gems as well 🤔). This PR was also failing due to not updating sidekiq-cron. So this new PR will do this as well.
Another issue was we now using
redis-clientgem, which also uses theRedisClientconstant name (they use a class, we used a module). We are now clashing on this. Judging from the PR that brought in our version ofRedisClient#2745, the intention wasn't to shadow this gem at all, so we can just rename it to something better.So just renamed this to be
Jupiter::Redisand also wrapped it in the connection pool (we already have the gem in our application)? I assume this is fine? Not sure if there was a reason not to use connection pool in the past 🤔. Then with the new renamed module, I updated all the usages of this.Sidekiq seems to be working fine locally (all tests are passing) and any job I ran seem to run fine:
Ran all the cron jobs we have as well and these all succeed fine:
So hopefully with being on Sidekiq 7.0.4+ this issue #3014 should be fixed now as well