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

Try to enqueue outgoing jobs in another worker #71

Merged
merged 2 commits into from
Oct 10, 2019

Conversation

suonlight
Copy link
Contributor

Summary

This commit is trying to fix issue RedisMutex::LockError on
#57.
Whenever we have the error, it simply enqueues another worker.
At the begin of the worker, if the job is succedeed, we simply call
enqueue_outgoing_jobs again.

# Summary

This commit is trying to fix issue RedisMutex::LockError on
chaps-io#57.
Whenever we have the error, it simply enqueues another worker.
At the begin of the worker, if the job is succedeed, we simply call
`enqueue_outgoing_jobs` again.
lib/gush/worker.rb Outdated Show resolved Hide resolved
@pokonski
Copy link
Contributor

The one problem I see with this approach is: if LockError occurs on the N-th job of jobs.outgoing inside the loop, the job will retry enqueuing even the jobs that were correctly enqueued on the previous run and we might have duplicate executions of downstream jobs.

Another approach to solve it would be to move the body of the block of the loop to a separate job (ScheduleOutgoingJob) and move the RedisMutex block into it. That way when it fails, ActiveJob/Sidekiq can retry that job on its own a bunch of times.

@suonlight
Copy link
Contributor Author

If the outgoing jobs were correctly enqueued, then the retry job just wastes time to loop on the outgoing jobs without doing anything. Because we have a check ready_to_start?.

The approach ScheduleOutgoingJob looks promising, I will try it by another PR.

@pokonski
Copy link
Contributor

Good point, so your solution might be enough as it is :)

@pokonski
Copy link
Contributor

Let's see if this works for people, if not - we can revisit with a separate ScheduleOutgoingJob

@pokonski pokonski merged commit 682c7fe into chaps-io:master Oct 10, 2019
@pokonski
Copy link
Contributor

I'll try to release it today, if time allows, thanks for your first and very important contribution 🎆

@suonlight suonlight deleted the df/fix-redis-mutex-approach-2--57 branch October 10, 2019 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants