Use Sidekiq::Process #12
Conversation
This makes the code easier to read and relies on official Sidekiq APIs, which are less likely to break. It also only looks at the current process to see, whether it is still processing jobs, not any quieted process. `identity` is inherited from `Sidekiq::Util`. However, it relies on APIs added in Sidekiq 5
|
This would mean removing support for Sidekiq 4.X. This means that we'll have to update our stack (@ccyrille). In the mean time 5.0 is out since April 2017, so it looks pretty safe to upgrade. Otherwise, LGTM! For later reviewers, the sidekiq process doc is available here: https://github.com/mperham/sidekiq/wiki/API#processes. |
There was a problem hiding this comment.
@iGEL sorry it took me so long to come back to you ! Not so many people still use Sidekiq < 5 now and relying on official APIs is always a good idea, so I think we can move along with that..
However tests don't pass anymore, they would need a little refactoring before we can merge. Could you make this happen ? I of course will be much more reactive on my next review 😉
|
Since the PR author is not responding, we're gonna stabilize the code in a new branch. Hence, this PR should be closed. @ccyrille |
|
@BuonOmo In fact, I couldn't find a way to pull the branch locally. I followed these steps to be able to do it : https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally#modifying-an-inactive-pull-request-locally edit : the author's contributions would not be overwritten |
|
Maybe he unchecked this option, that is unfortunate though :/ |
This makes the code easier to read and relies on official Sidekiq APIs, which are less likely to break. It also only looks at the current process to see, whether it is still processing jobs, not any quieted process.
identityis inherited fromSidekiq::Util.However, it relies on APIs added in Sidekiq 5