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

Add cleanup implementation to filesystem backend #6919

Merged
merged 3 commits into from
Aug 21, 2021

Conversation

eprigorodov
Copy link
Contributor

Description

Filesystem back-end stores task results in files and currently has no cleanup() method. Without other ways of maintenance (e.g. logrotate or systemd-tmpfiles) that will eventually overflow file system. This PR adds cleanup() implementation, which we use in our production.

@codecov
Copy link

codecov bot commented Aug 19, 2021

Codecov Report

Merging #6919 (ab9646b) into master (16959cd) will decrease coverage by 0.00%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6919      +/-   ##
==========================================
- Coverage   89.21%   89.21%   -0.01%     
==========================================
  Files         138      138              
  Lines       16678    16692      +14     
  Branches     2107     2112       +5     
==========================================
+ Hits        14880    14892      +12     
  Misses       1577     1577              
- Partials      221      223       +2     
Flag Coverage Δ
unittests 89.21% <85.71%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
celery/backends/filesystem.py 94.28% <85.71%> (-2.15%) ⬇️

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 16959cd...ab9646b. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Aug 19, 2021

This pull request introduces 2 alerts when merging 4660141 into 16959cd - 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.

if you can try to improve the code coverage that would be really great

in backends.filesystem.FilesystemBackend.cleanup()
@lgtm-com
Copy link

lgtm-com bot commented Aug 19, 2021

This pull request introduces 2 alerts when merging 9bdf8ae into 16959cd - view on LGTM.com

new alerts:

  • 2 for Wrong name for an argument in a call

@eprigorodov
Copy link
Contributor Author

Hi @auvipy, thank you for fast response and explanation. The pipelines in Celery project are great, but I need some advise on how to fix the coverage.

  1. The test_cleanup() is marked to be skipped on Windows. That is done because test makes a small sleep to divide expired and non-expired sets of task result flies. To make that work reliably on Windows + FAT the sleep time (according to the docs) should be longer than two seconds, which is not a good thing for testing, I believe.

If no unit testing is run on Windows + FAT then the test can be enabled on Windows as is. Otherwise the only options are either add longer delay or ignore decreasing coverage. Please advise.

  1. To fix new coverage finding I would need to add a third commit. Is that fine or should all commits better be squashed into one?

Thank you

@auvipy
Copy link
Member

auvipy commented Aug 20, 2021

we can sqash commits while merge. i am ok with the current coverage. can you just check on pypy failures?

@eprigorodov
Copy link
Contributor Author

From what I can understand:

  collecting ... ERROR: InvocationError for command 'D:\a\celery\celery\.tox\pypy3-unit\Scripts\pytest.EXE' --maxfail=10 -v --cov=celery --cov-report=xml --cov-report term (exited with code 3221225477)

exit code 3221225477 means segfault. And here is an issue that looks quite relevant: pytest-dev/pytest#6419.

I'll try to rewrite with os.listdir() and os.stat().

due to possible problems when testing under pypy-3.7, windows-2019
(pytest-dev/pytest#6419)
@auvipy auvipy merged commit 3ef5b54 into celery:master Aug 21, 2021
@eprigorodov
Copy link
Contributor Author

hi @auvipy, thank you for reviewing and approval.

One last question: we have to maintain a number of boxes with Debian 9 -> Python 3.5 -> Celery 4.4. Can I somehow make a PR for backporting this feature to branch "4.x"?

@auvipy
Copy link
Member

auvipy commented Aug 23, 2021

4.x is EOL. you should use pyenv to upgrade your python and should not rely on systems python.

jeyrce pushed a commit to jeyrce/celery that referenced this pull request Aug 25, 2021
* Add cleanup implementation to filesystem backend

* improve unit test coverage

in backends.filesystem.FilesystemBackend.cleanup()

* replace os.scandir() with os.listdir()

due to possible problems when testing under pypy-3.7, windows-2019
(pytest-dev/pytest#6419)
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