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

"Reentrant" version, that allows a celery.bin.celery.main worker to run multiple times #866

Merged
merged 6 commits into from
Aug 10, 2018

Conversation

alanjds
Copy link
Contributor

@alanjds alanjds commented May 9, 2018

The actual version does not allow celery.bin.celery.main to run, raise a SystemExit, catch it and then run again. (See: alanjds/celery-serverless@a23f79d#diff-b4826aeb276ca699cf1adda1903fe3eaR55)

To allow it, the kombu.async.hub.Hub.poller became a @Property that regenerates on access. And the Redis backend tries to recreate it before acessing, if needed.

Indeed, celery.worker.state.should_{stop,terminate} should be reset after every SystemExit for it to work, but this part is a patch not for Kombu repo ;) .
(See: alanjds/celery-serverless@a23f79d#diff-b4826aeb276ca699cf1adda1903fe3eaR57)

@codecov
Copy link

codecov bot commented May 9, 2018

Codecov Report

Merging #866 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #866      +/-   ##
==========================================
+ Coverage   86.26%   86.27%   +0.01%     
==========================================
  Files          63       63              
  Lines        6485     6491       +6     
  Branches      768      769       +1     
==========================================
+ Hits         5594     5600       +6     
  Misses        812      812              
  Partials       79       79
Impacted Files Coverage Δ
kombu/asynchronous/hub.py 78.19% <100%> (+0.5%) ⬆️

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 baf488b...48a8373. Read the comment docs.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

thanks for the patch, is it possible to improve the coverage of test?

@alanjds
Copy link
Contributor Author

alanjds commented May 11, 2018

To raise the coverage, I can think of a case closing the Redis pooler an then trying to reuse it again. Will take a look on what file it would fit...

@alanjds
Copy link
Contributor Author

alanjds commented Aug 6, 2018

@auvipy I am trying to bake a test case for "Redis got closed then reused again", but could not find a way: The actual tests never set MultiChannelPoller.poller = None, the precondition for the error to occur.

I could use some help here, if possible.

@alanjds
Copy link
Contributor Author

alanjds commented Aug 6, 2018

Looks like Travis failed only on Python 3.4 and pypy, for some unknown reason. Can you please issue a retry there please?

@georgepsarakis
Copy link
Contributor

@alanjds failed builds restarted.

Copy link
Member

@thedrow thedrow left a comment

Choose a reason for hiding this comment

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

LGTM

@auvipy auvipy added this to the 4.3 milestone Aug 10, 2018
@auvipy auvipy merged commit 073001e into celery:master Aug 10, 2018
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.

4 participants