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.

Sign up for GitHub

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

Jump to bottom

fix redis broker quit very slow #443

Closed
wants to merge 5 commits into from
Closed

fix redis broker quit very slow #443

wants to merge 5 commits into from

Conversation

pavlelee
Copy link

@pavlelee pavlelee commented Jul 18, 2019 •
edited
Loading

#439

  1. Use select default is more logical
  2. Add NormalTasksPollPeriod param to control redis BLPOP timeout, depending on your business scenario choice suitable value.

@pavlelee pavlelee closed this Jul 19, 2019
@pavlelee pavlelee reopened this Jul 19, 2019
@codecov
Copy link

codecov bot commented Jul 19, 2019 •
edited
Loading

Codecov Report

Merging #443 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #443   +/-   ##
=======================================
  Coverage   47.99%   47.99%           
=======================================
  Files          29       29           
  Lines        2642     2642           
=======================================
  Hits         1268     1268           
  Misses       1234     1234           
  Partials      140      140
Impacted Files Coverage Δ
v1/common/redis.go 0% <ø> (ø) ⬆️
v1/config/config.go 80% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef7d07c...bdd1db5. Read the comment docs.

@pavlelee pavlelee closed this Jul 19, 2019
@pavlelee pavlelee reopened this Jul 19, 2019
@charleswhchan
Copy link
Contributor

@Liv1020 : could you please add a test as well? This would help pre ent the issue from reappearing again in future. Thanks

@pavlelee
Copy link
Author

@charleswhchan ok, I already add test case, but i delete it after see integration-tests

@charleswhchan
Copy link
Contributor

Thanks @Liv1020 . My point was that the existing tests did catch this issue before, so it would be nice to add a test a regression again in future. 😃

@pavlelee pavlelee closed this Jul 22, 2019
@pavlelee pavlelee reopened this Jul 22, 2019
@pavlelee pavlelee closed this Jul 22, 2019
@pavlelee pavlelee reopened this Jul 22, 2019
@pavlelee
Copy link
Author

@charleswhchan Already add to integration-tests

@charleswhchan
Copy link
Contributor

Thanks @Liv1020!

@charleswhchan
Copy link
Contributor

@Liv1020, @RichardKnop : I tested the fix and it is working for me.

@charleswhchan charleswhchan mentioned this pull request Aug 3, 2019
Merged
@charleswhchan
Copy link
Contributor

@Liv1020 : I made a PR awhile ago and the fix is now included in v.1.6.8. Can you give it a try?

ankurcha pushed a commit to ankurcha/machinery that referenced this pull request Sep 1, 2019
@ankurcha
Fix RichardKnop#439 race condition and slow shutdown with worker
fc68805 
1. When launching Redis worker and then shutdown immediately, StopConsuming() could try to close(b.StopChan) BEFORE b.StopChan is initialized inside StartConsuming().
Solution: Initialize field when object is being initialized.

2. When using Redis broker, nextTaks() usings BLPOP can block for a long time.
Solution: Make timeout value configurable.

See also: PR RichardKnop#406, RichardKnop#443
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pavlelee @charleswhchan @RichardKnop