Add Sidekiq 6.1 support
Context
Adding support for Sidekiq 6.0 is a piece of cake as there were no changes that affect the gitlab-sidekiq-fetcher
.
However, version 6.1
introduced a breaking change. See merge request here
TL;DR;
There is a :fetch
option what allows to configure a custom fetcher. It's a class that should implement an interface that Sideqik is using to fetching jobs from Redis.
In < 6.1
the :fetch
option was expecting a class reference and sidekiq was initializing the class itself. Since 6.1
the fetch
option must be an instance of such a fetcher class.
Initially, it seemed to be easy: just changing class methods to instance methods. That did the job and integration tests started to be green.
I also wanted this gem to still support older versions of sidekiq (5 < x < 7
). I first run the gem with sidekiq 6.0
to check if all works and it worked well. Then I run with sidekiq 6.1
and I started to observe the following error in Sidekiq logs:
FiberError: fiber called across threads Enumerator next
and the traceback was showing this line.
I send a good amount of time trying to figure out why integration tests don't throw the same error. I was thinking that the app I'm testing has a different setup or initializes the threads differently. Only after deep debugging, I realized that the error only appears when there are multiple queues.
After changing integration tests to run multiple queues they guided me to the root cause.
ReliableFetch strategy issue
For the ReliableFetch
fetcher we use Enumerator
instance to iterate through the queues. Enumerator is created in during class initialization. It is fine for Sidekiq < 6.1 because the fetch class is initialized inside every Thread and is owned and used only by this Thread.
In 6.1
initialization is done once before creating threads and one instance is passed to all the Threads.
We can't use Enumerator
in such a case as it is not thread-safe. First I've tried using Queue
(as this is advertised as thread-safe replacement for Enumerator but it won't help here because it is cross-thread and as a result, all threads have access to the same iterator. Also, w need an infinite (cycle) iterator and it's hard to achieve with Queue
.
With @vsizov we decided to go simple and get rid of the Enumerator
and instead shuffle
the array of queues inside every retrive_unit_of_work
call (for not-ordered queues). It should be fine for avoiding queues starvation. (This is also used by default sideqik
fetcher and SemiRiableFetch. (Actually the Enumerator wasn't essential and intentional)
Sidekiq version support
This MR releases new version of the gem 0.7.0
and since this version only Sidekiq >=6.1
will be supported. If someone would like to use it with older sidekiq we recommend to use version 0.5
of this gem. This is because of the braking changes described above.
Changes
- Added support for sidekiq
>= 6.1
by replacing class methods with instance ones and setup method - Dropped support for sidekiq
< 6.1
and added a note about that to theREADME
- Changed the implementation of
retrive_unit_of_work
inReliableFetch
strategy as described above to by replacing Enumerator with an Array (shuffled or ordered) inReliableFetch
In tests
- Changed integration tests and spec to run against
Sidekiq ~> 6.1
- Made integration tests running workers with multiple queues (There is a lot of code that is operating on multiple queues and in real life, you use multiple queues and I thought that it would be good for the integration tests to cover that - it also helped me to catch a bug with FiberError)
- Simplified how we get job id (
jid
) inReliabilityTestWorker