Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Basic implementation of backup media store #2538
Conversation
erikjohnston
added some commits
Oct 12, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
Some checks were not successful
turt2live
commented
Oct 12, 2017
|
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 |
turt2live
commented
Oct 12, 2017
|
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. |
erikjohnston
added some commits
Oct 12, 2017
Some checks were not successful
All checks have failed
Some checks were not successful
Some checks were not successful
Some checks were not successful
|
@turt2live its possible, though I'm afraid its not something I'll be doing now, though it'd be neat |
Some checks were not successful
erikjohnston
assigned
richvdh
Oct 12, 2017
| 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): |
| 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): |
| 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): |
| + self.filepaths = MediaFilePaths(self.primary_base_path) | ||
| + | ||
| + self.backup_base_path = None | ||
| + if hs.config.backup_media_store_path: |
| @@ -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): |
| + shutil.copyfileobj(source, f) | ||
| + | ||
| + if close_source: | ||
| + source.close() |
richvdh
Oct 12, 2017
Member
Though I have to say that having the close happen in the same function as the open generally seems less confusing to me
erikjohnston
Oct 13, 2017
Owner
Yes, it is very sad, but I don't know how else to do it since the writing to the backup can happen asynchronously, and so may last longer than the context which calls it.
erikjohnston
Oct 13, 2017
Owner
(The other option is to rely on the GC closing the file, but that doesn't sound great either)
erikjohnston
Oct 13, 2017
Owner
I guess we could copy from the file we wrote into the primary media store, but thats a bit sad making in terms of having to reload that file back into memory.
| + source.close() | ||
| + | ||
| + @defer.inlineCallbacks | ||
| + def write_to_file(self, source, path): |
richvdh
Oct 12, 2017
Member
can you call this something like write_to_file_and_backup? I kept confusing myself over whether it did the backup or not.
| + | ||
| + Args: | ||
| + source: A file like object that should be written | ||
| + path: Relative path to write file to |
| + path: Relative path to write file to | ||
| + | ||
| + Returns: | ||
| + string: the file path written to in the primary media store |
| + string: the file path written to in the primary media store | ||
| + """ | ||
| + fname = os.path.join(self.primary_base_path, path) | ||
| + self._makedirs(fname) |
| + self._makedirs(fname) | ||
| + | ||
| + # Write to the main repository | ||
| + yield preserve_context_over_fn( |
richvdh
Oct 12, 2017
Member
preserve_context_over_fn is broken. I think you want make_deferred_yieldable.
Can I suggest that you write a write_file_asynchronously which factors out this and the other instance below?
| + def copy_to_backup(self, source, path): | ||
| + """Copy file like object source to the backup media store, if configured. | ||
| + | ||
| + Will close source after its done. |
| + # We can either wait for successful writing to the backup repository | ||
| + # or write in the background and immediately return | ||
| + if self.synchronous_backup_media_store: | ||
| + yield preserve_context_over_fn( |
| + ) | ||
| + else: | ||
| + source.close() | ||
| + | ||
| @defer.inlineCallbacks | ||
| def create_content(self, media_type, upload_name, content, content_length, |
richvdh
Oct 12, 2017
Member
if we're changing the type of content, let's take the opportunity to doc it.
Especially if we're going to close the stream (which, as above, I am unconvinced about).
| + | ||
| + t_len = os.path.getsize(output_path) | ||
| + | ||
| + yield self.store.store_local_thumbnail_rel( |
richvdh
Oct 12, 2017
Member
_rel looks incorrect here, and makes me worry that this hasn't been well tested.
| - 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, |
| - 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 |
richvdh
Oct 12, 2017
Member
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 |
erikjohnston
added some commits
Oct 13, 2017
All checks have passed
Some checks were not successful
Some checks were not successful
Some checks were not successful
erikjohnston
assigned
erikjohnston
and unassigned
richvdh
Oct 13, 2017
| """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 |
| if t_byte_source: | ||
| - output_path = yield self.write_to_file( | ||
| + t_width, t_height = t_byte_source.dimensions |
richvdh
Oct 13, 2017
Member
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.
erikjohnston
Oct 13, 2017
Owner
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.
erikjohnston
Oct 13, 2017
Owner
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) |
erikjohnston
added some commits
Oct 13, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
| + 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( |
richvdh
Oct 13, 2017
Member
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( |
erikjohnston
added some commits
Oct 13, 2017
Some checks were not successful
erikjohnston commentedOct 12, 2017
No description provided.