-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Avoid storing URL cache files in storage providers #10911
Changes from all commits
bd39f32
258d566
04f27a7
0736d1b
2366cfb
b81ccf8
f023c74
b551888
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Avoid storing URL cache files in storage providers. Server admins may safely delete the `url_cache/` and `url_cache_thumbnails/` directories from any configured storage providers to reclaim space. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,6 +93,11 @@ async def store_file(self, path: str, file_info: FileInfo) -> None: | |
if file_info.server_name and not self.store_remote: | ||
return None | ||
|
||
if file_info.url_cache: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious why you chose to do this here instead of where we make the URL previews? My guess is to ensure we don't break this if we add another way of having URL previews? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A little bit, but mostly for existing code. The call hierarchy gets pretty messy above The choice between |
||
# The URL preview cache is short lived and not worth offloading or | ||
# backing up. | ||
return None | ||
|
||
if self.store_synchronous: | ||
# store_file is supposed to return an Awaitable, but guard | ||
# against improper implementations. | ||
|
@@ -110,6 +115,11 @@ async def store() -> None: | |
run_in_background(store) | ||
|
||
async def fetch(self, path: str, file_info: FileInfo) -> Optional[Responder]: | ||
if file_info.url_cache: | ||
# Files in the URL preview cache definitely aren't stored here, | ||
# so avoid any potentially slow I/O or network access. | ||
return None | ||
|
||
# store_file is supposed to return an Awaitable, but guard | ||
# against improper implementations. | ||
return await maybe_awaitable(self.backend.fetch(path, file_info)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we're making this change, is it just for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be so we can use it in tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's for the tests