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

Prevent worker to send expired revoked items upon hello command #6975

Merged
merged 2 commits into from
Oct 1, 2021

Conversation

olii
Copy link
Contributor

@olii olii commented Sep 29, 2021

This PR solves the issue when a newly spawned worker receives a lot of old and expired revocations.
There is no reason to send old revocations that are already expired.

In bigger systems that rely on revocations and use a much larger LimitedSet this is a problem as the hello response may be hundreds of megabytes large. Receiving such a large message from other workers takes a lot of time and also stresses RabbitMQ (the broker we use). This is also a way to reclaim some memory back in case there are no longer active revocations.

Let me know what do you think about it.

@lgtm-com
Copy link

lgtm-com bot commented Sep 29, 2021

This pull request introduces 2 alerts when merging dcb99db into 71ed45d - view on LGTM.com

new alerts:

  • 2 for Wrong name for an argument in a call

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.

pleasecheck test failures

@olii
Copy link
Contributor Author

olii commented Sep 30, 2021

Alerts are not caused by my PR. They are already in master.

@codecov
Copy link

codecov bot commented Sep 30, 2021

Codecov Report

Merging #6975 (e0228d4) into master (71ed45d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6975   +/-   ##
=======================================
  Coverage   89.25%   89.25%           
=======================================
  Files         138      138           
  Lines       16729    16730    +1     
  Branches     2120     2120           
=======================================
+ Hits        14931    14932    +1     
  Misses       1575     1575           
  Partials      223      223           
Flag Coverage Δ
unittests 89.24% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
celery/worker/control.py 95.52% <100.00%> (+0.01%) ⬆️

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 71ed45d...e0228d4. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Sep 30, 2021

This pull request introduces 2 alerts when merging e0228d4 into 71ed45d - view on LGTM.com

new alerts:

  • 2 for Wrong name for an argument in a call

@auvipy auvipy added this to the 5.2 milestone Oct 1, 2021
@auvipy auvipy merged commit b0ecc35 into celery:master Oct 1, 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.

None yet

3 participants