Skip to content
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

Start new orphan reaper process if orphan reaper is not running #604

Merged
merged 5 commits into from
Jun 4, 2021

Conversation

AlexFlint73
Copy link
Contributor

If a sidekiq process that runs orphan reaper stops unexpectedly and the new process is up within reaper_interval, new orphan reaper won't start.

We've faced a rare issue, when the sidekiq process, that went down, was running both orphan reaper and a job, that needs to be unique. This led to leaving an orphaned lock and the job became stuck.

I believe, this PR can fix this. Each sidekiq process will run reaper ressurector, that starts a new orphan reaper if it looks like there is no reaper process running.

@mhenrixon
Copy link
Owner

This is brilliant, the only thing that might be improved would be that there seems to be a little duplication.

I haven't had time to properly review the code on my computer but I'll get to it as soon as I can.

It would be nice to avoid the additional thread but it could be considered an optional feature.

Thank you

@AlexFlint73
Copy link
Contributor Author

Thanks a lot for your reply.

I've thought on how an additional thread can be avoided. However, it looks like a running thread is needed if a new reaper should be started on the fly.

Another possible solution can be running an instance of orphan reaper on each sidekiq process and use mutex in redis to make sure only one reaper checks locks during single iteration. Such a solution will cost 1 thread less.

As to duplication, I see what you mean. Will work on that.

Copy link
Owner

@mhenrixon mhenrixon left a comment

Choose a reason for hiding this comment

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

Some initial feedback for you :)

lib/sidekiq_unique_jobs.rb Outdated Show resolved Hide resolved
lib/sidekiq_unique_jobs/orphans/manager.rb Outdated Show resolved Hide resolved
lib/sidekiq_unique_jobs/orphans/manager.rb Outdated Show resolved Hide resolved
@mhenrixon mhenrixon merged commit 92c8d36 into mhenrixon:master Jun 4, 2021
@benedikt benedikt mentioned this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants