Prevent Cache.download() deleting an expired file before knowing that it can download a new version of the file
#7265
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
samish55
force-pushed
the
main
branch
2 times, most recently
from
4f5b086
to
d69fa1b
Compare
|
Thanks for the code. I think ideally we need a unit test to make sure this behavior is working. It is going to be tricky to do as you will need to monkey patch several calls (I think) to fake the download and make it fail. |
nabobalis
changed the title
Cache.download() deleting an expired file before knowing that it can download a new version of the file
nabobalis
added
data
nabobalis
force-pushed
the
main
branch
2 times, most recently
from
0e03f22
to
c188102
Compare
nabobalis
added
Minor Change
nabobalis
previously approved these changes
Apr 24, 2024
nabobalis
reviewed
Apr 25, 2024
| @@ -40,56 +39,60 @@ def download(self, urls, namespace='', redownload=False): | |||
| """ | |||
| Downloads the files from the urls. | |||
|
|
|||
| The overall flow of this function is: | |||
There was a problem hiding this comment.
This is now just in the code as a comment instead of in both places.
nabobalis
reviewed
Apr 25, 2024
Comment on lines
+79
to
+80
| if cache_details and (redownload or self._has_expired(cache_details)): | ||
| self._storage.delete_by_key('url', cache_details['url']) |
There was a problem hiding this comment.
I think there is logic problem here:
- We do not remove any old files from disk
- If the file changes on disk, we could remove it here but then it will break logic in the manager as we need to hash the file a second time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
I've attempted to handle issue #7249. The expired file won't be deleted until the file is successfully downloaded and if the download fails(for some reason) an error warning is emitted about working with an expired file in cache.
Fixes #7249