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

cache: added clear_expired_cache (#1055) #1061

Closed
wants to merge 18 commits into from
Closed

cache: added clear_expired_cache (#1055) #1061

wants to merge 18 commits into from

Conversation

pl-marasco
Copy link
Contributor

As discussed in #1055 time to time there is the need to keep data locally and remove them after a defined time.
.clear_expired_cache() add the possibility to remove only the cached data that overpass the defined time.

martindurant
martindurant reviewed Oct 4, 2022
View reviewed changes
Copy link
Member

@martindurant martindurant 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 this and the nice comprehensive test.

I have just a couple of small clarification questions.

fsspec/implementations/cached.py Outdated Show resolved Hide resolved
fsspec/implementations/cached.py Show resolved Hide resolved
@martindurant
Copy link
Member

Sorry, some blank lines around in there - please run black on here. I could apparently commit my suggested docstring format change, but I can't push to your master branch :)

@pl-marasco
Copy link
Contributor Author

Don't even say sorry as is my fault. I run black on the test component and I forgot to do the same on the cached.py

@martindurant
Copy link
Member

Would you mind investigating the failure? I am not convinced that the test is correct, and your popping from the cache might actually be the right thing to do. I am uncertain.

@martindurant
Copy link
Member

ping

@pl-marasco
Copy link
Contributor Author

I just back from COVID, I'll restart working on it today.
I had time to though a lot about the approach and I'm considering taking a completely different approach. I most probably will not use the 'save' method and use threads to manage the delete (if u agree on this). From my perspective threads are needed in case of processing lots of files or big files.

@martindurant
Copy link
Member

just back from COVID

you and me both :)

I am not in general favour of using threads here, because fsspec is commonly used from within multithreaded settings anyway. It's OK if the process takes a while, since the user has explicitly asked for it. Furthermore, os operations like rm cannot actually operate in parallel anyway.

ptrba and others added 15 commits November 2, 2022 15:58
@ptrba @pl-marasco
explicitly wrap flush LocalFileOpener (#1070)
c6c54df 
* explicitly wrap flush and name of LocalFileOpener

* revert explicit wrapping of local name

Co-authored-by: Peter Barmettler <peter.barmettler@raccoonworks.ch>
@ghislainp @pl-marasco
solve a bug when the 'write' method in an implementation does not ret… (
f5acc7f 
#1072)

* solve a bug when the 'write' method in an implementation does not return the actual length of written data (the case of SFTP with paramiko)

* add a test for put_file

* typo

* rpath must be a new file

* remove extra tabs
@ianthomas23 @pl-marasco
Update readme to reflect python versions in tox (#1076)
4199686
@martindurant @pl-marasco
Reference improvements (#1063)
a58dc46 
* Always populate reference fss keys

* consolidate reference requests

* Implemented

* Allow start and stop in reference cat_file

* Surface pars and add docs
@martindurant @pl-marasco
changelog for release (#1077)
f15a356
@edrodrigues-ag @pl-marasco
avoid ESA snappy (#1079)
96abe5c
@ianthomas23 @pl-marasco
Add topdown kwarg to AbstractFileSystem.walk() (#1081)
56fd722 
* Add topdown kwarg to AbstractFileSystem.walk()

* Linting
@martindurant @pl-marasco
Keep HTTP URLs as str until encoding required (#1085)
1d5776e 
* Keep HTTP URLs as str until encoding required

* add py dep
@ianthomas23 @pl-marasco
Pin docutils so that sphinx renders lists correctly (#1084)
624f671
@martindurant @pl-marasco
Sort when merging offsets for referenceFS (#1087)
6bbbb8f
@maximfridental @pl-marasco
Implement recursive parameter in the get method of SFTP (#1082)
94536c7 
* Implement recursive parameter in the get method of SFTP implementation and enable passing urls to it

* Simplify implementation of recursive get for SFTP

* Improve test_sftp::test_get_dir

* Improve imports
@eamanu @pl-marasco
Use test serve with explicit 127.0.0.1 url (#1088)
ab63103 
This patch remove the use of localhost in the test server to avoid
issues with host configured only with ipv6.

Fix: #927
@leoleoasd @pl-marasco
Support append mode for ArrowFSWrapper (#1089)
e57b24e 
* Support append mode for ArrowFSWrapper

* Add test for append in HDFS
@pl-marasco
cache: added clear_expired_cache (#1055)
a7530fb
@pl-marasco
Merge branch 'fsspec:master' into master
4da82da
@pl-marasco pl-marasco closed this Nov 2, 2022
@pl-marasco
Copy link
Contributor Author

As the approach is quite different from the original one and as it has been hanging for more than a month I would prefer to create a new pull request based on the new approach and on the latest version

@martindurant
Copy link
Member

Thanks for sticking with it, @pl-marasco

@pl-marasco pl-marasco mentioned this pull request Nov 2, 2022
Merged
martindurant pushed a commit that referenced this pull request Nov 4, 2022
@pl-marasco
cache: added clear_expired_cache (#1055 - #1061) (#1092)
39efd7f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martindurant martindurant martindurant left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@pl-marasco @martindurant @ptrba @ghislainp @ianthomas23 @edrodrigues-ag @maximfridental @eamanu @leoleoasd