Basic implementation of backup media store #2538
Conversation
|
Is the intention for this to have some kind of repo failover? |
|
Yup, just having all files backed up somewhere else so that it can be manually failed over if necessary |
|
Would it be possible to have that be automatic in a way, printing a plethora of warnings? Probably doesn't need to be all that complex: try reading from primary, if that fails, try the backup, continue on with whatever result. |
|
@turt2live its possible, though I'm afraid its not something I'll be doing now, though it'd be neat |
| file_id[0:2], file_id[2:4], file_id[4:], | ||
| file_name | ||
| ) | ||
|
|
||
| remote_media_thumbnail = _wrap_in_base_path(remote_media_thumbnail_rel) | ||
|
|
||
| def remote_media_thumbnail_dir(self, server_name, file_id): |
There was a problem hiding this comment.
should this get a _rel for consistency?
There was a problem hiding this comment.
This actually returns absolute paths (now..)
| media_id[0:2], media_id[2:4], media_id[4:], | ||
| ) | ||
|
|
||
| url_cache_filepath = _wrap_in_base_path(url_cache_filepath_rel) | ||
|
|
||
| def url_cache_filepath_dirs_to_delete(self, media_id): |
There was a problem hiding this comment.
_rel? or at least clarify it in the comments?
There was a problem hiding this comment.
This actually returns absolute paths (now..)
| media_id[0:2], media_id[2:4], media_id[4:], | ||
| file_name | ||
| ) | ||
|
|
||
| url_cache_thumbnail = _wrap_in_base_path(url_cache_thumbnail_rel) | ||
|
|
||
| def url_cache_thumbnail_directory(self, media_id): |
There was a problem hiding this comment.
This actually returns absolute paths (now..)
| self.filepaths = MediaFilePaths(self.primary_base_path) | ||
|
|
||
| self.backup_base_path = None | ||
| if hs.config.backup_media_store_path: |
There was a problem hiding this comment.
I'm missing why this is needed?
| @@ -87,18 +96,77 @@ def _makedirs(filepath): | |||
| if not os.path.exists(dirname): | |||
| os.makedirs(dirname) | |||
|
|
|||
| @staticmethod | |||
| def _write_file_synchronously(source, fname, close_source=False): | |||
| source.close() | ||
|
|
||
| @defer.inlineCallbacks | ||
| def write_to_file(self, source, path): |
There was a problem hiding this comment.
can you call this something like write_to_file_and_backup? I kept confusing myself over whether it did the backup or not.
| if r_method == "scale": | ||
| t_width, t_height = thumbnailer.aspect(r_width, r_height) | ||
| scales.add(( | ||
| min(m_width, t_width), min(m_height, t_height), r_type, |
There was a problem hiding this comment.
isn't the deduplication here important?
| t_len = thumbnailer.crop(t_path, t_width, t_height, t_type) | ||
| local_thumbnails.append(( | ||
| media_id, t_width, t_height, t_type, t_method, t_len | ||
| r_width, r_height, r_method, r_type, t_byte_source |
There was a problem hiding this comment.
Isn't this going to result in us having all the thumbnails in memory at once? can't we do the write_to_file here (and for that matter, the store_local_thumbnail)?
| if r_method == "scale": | ||
| t_width, t_height = thumbnailer.aspect(r_width, r_height) | ||
| scales.add(( | ||
| min(m_width, t_width), min(m_height, t_height), r_type, |
| ]) | ||
|
|
||
| remote_thumbnails.append(( | ||
| r_width, r_height, r_method, r_type, t_byte_source |
| """Write `source` to the on disk media store, and also the backup store | ||
| if configured. | ||
|
|
||
| Will close source once finished. | ||
|
|
||
| Args: | ||
| source: A file like object that should be written | ||
| path: Relative path to write file to | ||
| path(str): Relative path to write file to |
There was a problem hiding this comment.
for future ref, I prefer a space between path and (str), but no biggie
|
|
||
| if t_byte_source: | ||
| output_path = yield self.write_to_file( | ||
| t_width, t_height = t_byte_source.dimensions |
There was a problem hiding this comment.
I'm confused.
t_byte_sourceis the result from_generate_thumbnail_generate_thumbnaildoesn't document its return type (grr) but apparently it's the result fromthumbnailer.croporthumbnailer.scalecropandscalereturn an ordinary BytesIO (at least according to their docs)- BytesIO doesn't have a
dimensionsattribute
either something's lying, or you (still) haven't tested this. Or I'm being a total crank.
There was a problem hiding this comment.
why is this change happening, anyway?
There was a problem hiding this comment.
Yes, this was a spurious change, sowwy.
In other news, I've been looking at getting sytest to test both dynamic and static config, and it turns out that this dynamic code path doesn't work at all on master....
I think we should look into fixing/removing this functionality, but that should probably wait for a separate PR.
There was a problem hiding this comment.
In other news, I've been looking at getting sytest to test both dynamic and static config, and it turns out that this dynamic code path doesn't work at all on master....
After massaging sytest a bit I've exercised that code path in a way that returns 200, so it seems no more broken than before, at least.
|
|
||
| if t_byte_source: | ||
| output_path = yield self.write_to_file( | ||
| t_width, t_height = t_byte_source.dimensions |
| t_width, t_height = thumbnailer.aspect(r_width, r_height) | ||
| t_width = min(m_width, t_width) | ||
| t_height = min(m_height, t_height) | ||
| thumbnails[(t_width, t_height)] = (r_method, r_type) |
There was a problem hiding this comment.
shouldn't r_type be in the key rather than the value?
| Args: | ||
| source: A file like object to be written | ||
| fname (str): Path to write to | ||
| close_source (bool): Whether to close source after writing |
| )) | ||
|
|
||
| if t_byte_source: | ||
| output_path = yield self.write_to_file_and_backup( |
There was a problem hiding this comment.
looks like t_byte_source isn't being closed any more. Needs more with or finally.
| )) | ||
|
|
||
| if t_byte_source: | ||
| output_path = yield self.write_to_file_and_backup( |
No description provided.