Refactor MediaRepository to separate out storage #2767

erikjohnston · 2018-01-09T16:09:21Z

The end intention here is:

Have a local FS cache of media (which by default has everything in it)
Allow specifying additional StorageProviders that can be used to fetch media from, and optionally upload to.

In the future we should make these behaviours configurable through config options, as well as keeping track of last access time for local as well as remote media.

We should also look into making the MediaStorage layer aware of the deletion of old content, as currently that is still done by MediaRepository/PreviewUrlResource. Otherwise, only the MediaStorage and StorageProvider classes should care where things are on disk.

erikjohnston · 2018-01-09T16:18:30Z

This is a long long PR, though a lot of it is just adding new classes. I can split this up into separate PRs, but that would break the backup media repo stuff (as that's not added back in until the last commit)

erikjohnston · 2018-01-09T16:21:59Z

synapse/rest/media/v1/media_storage.py

+            with media_storage.store_into_file(info) as (f, fname, finish_cb):
+                # .. write into f ...
+                yield finish_cb()
+        """


I'm really not ecstatic by this API shape TBH, but I'm somewhat keen to ensure that the open file gets cleaned up correctly.

richvdh · 2018-01-11T10:45:34Z

synapse/rest/media/v1/media_repository.py

@@ -1,5 +1,6 @@
 # -*- coding: utf-8 -*-
 # Copyright 2014-2016 OpenMarket Ltd
+# Copyright 2018 New Vecotr Ltd


richvdh · 2018-01-11T10:49:07Z

synapse/rest/media/v1/media_repository.py

+    @defer.inlineCallbacks
+    def get_local_media(self, request, media_id, name):
+        """Responds to reqests for local media, if exists, or returns 404.
+        """


docs for param types and return type please

richvdh · 2018-01-11T10:51:19Z

synapse/rest/media/v1/download_resource.py

        else:
            yield self._respond_remote_file(
                request, server_name, media_id, name
            )

-    @defer.inlineCallbacks
-    def _respond_local_file(self, request, media_id, name):
-        media_info = yield self.store.get_local_media(media_id)


self.store is now redundant in the constructor (as are self.clock and self.version_string while you are there)

richvdh · 2018-01-11T11:00:08Z

synapse/rest/media/v1/download_resource.py

-        else:
-            file_path = self.filepaths.local_media_filepath(media_id)
-
-        yield respond_with_file(


Would have been helpful if you could have put the switch from respond_with_file to respond_with_responder in a separate commit to the switch from _respond_local_file to get_local_media, ftr.

richvdh · 2018-01-11T12:37:39Z

synapse/rest/media/v1/media_storage.py

+logger = logging.getLogger(__name__)
+
+
+class MediaStorage(object):


for future reference: it would also have been helpful if you could have built this class up over separate commits showing how it is used, rather than presenting the whole class as a fait accomplit that I then need to cross-reference to the commits where it is actually used. For example, in the first commit, add fetch_media and use it in get_local_media. Then add store_file in the next commit.

richvdh · 2018-01-11T14:27:09Z

synapse/rest/media/v1/download_resource.py

-        filesystem_id = media_info["filesystem_id"]
-        upload_name = name if name else media_info["upload_name"]
-
-        file_path = self.filepaths.remote_media_filepath(


filepaths now also redundant in the constructor

richvdh · 2018-01-11T14:30:06Z

synapse/rest/media/v1/media_repository.py

        self.dynamic_thumbnails = hs.config.dynamic_thumbnails
        self.thumbnail_requirements = hs.config.thumbnail_requirements

        self.remote_media_linearizer = Linearizer(name="media_remote")

        self.recently_accessed_remotes = set()

-        self.media_storage = MediaStorage(self.primary_base_path, self.filepaths)
+        # List of StorageProvider's where we should search for media and


StorageProviders

richvdh · 2018-01-11T14:31:09Z

synapse/rest/media/v1/media_repository.py

-        self.media_storage = MediaStorage(self.primary_base_path, self.filepaths)
+        # List of StorageProvider's where we should search for media and
+        # potentially upload to.
+        self.storage_providers = []


why is this a class field rather than a local var?

richvdh · 2018-01-11T14:40:52Z

synapse/rest/media/v1/media_storage.py

@@ -32,9 +32,10 @@ class MediaStorage(object):
    """Responsible for storing/fetching files from local sources.
    """

-    def __init__(self, local_media_directory, filepaths):
+    def __init__(self, local_media_directory, filepaths, storage_providers):


please doc the param types, especially the new one.

richvdh · 2018-01-11T14:44:57Z

synapse/rest/media/v1/storage_provider.py

+        pass
+
+
+class StorageProviderWrapper(StorageProvider):


I feel a bit like this could just go into FileStorageProviderBackend, but perhaps you have plans or reasons that won't work

These are options that will probably be used across any future storage providers, so it felt a bit silly to implement the same options across multiple ones (I have a S3 one lurking about).

For example, I can imagine in the future we might end up with config like:

storage_providers: - name: backup_fs store_thmbnails: false type: FileStorageProvider params: directory: /foo/bar

or something

…dia_storage_refactor

erikjohnston · 2018-01-12T13:32:47Z

I've made Responder a context manager, I can't decide whether it makes it nicer or not.

richvdh · 2018-01-12T13:50:46Z

I've made Responder a context manager, I can't decide whether it makes it nicer or not.

Nor can I really. I think it's probably an improvement.

richvdh · 2018-01-12T13:47:26Z

synapse/rest/media/v1/_base.py

        media_type (str): The media/content type.
-        file_size (int): Size in bytes of the media, if known.
+        file_size (int): Size in bytes of the media. If not known it should be None


so int|None I guess.

richvdh · 2018-01-12T13:52:28Z

synapse/rest/media/v1/media_repository.py

+            name (str|None): Optional name that, if specified, will be used as
+                the filename in the Content-Disposition header of the response.
+
+        Retruns:


richvdh · 2018-01-12T13:55:08Z

synapse/rest/media/v1/media_repository.py

+            name (str|None): Optional name that, if specified, will be used as
+                the filename in the Content-Disposition header of the response.
+
+        Retruns:


richvdh · 2018-01-12T13:56:44Z

synapse/rest/media/v1/media_repository.py

+
+        Args:
+            request(twisted.web.http.Request)
+            media_id (str)


sorry, but please could you doc media_id, here and all the other places it's used? it would help me be less confused about the difference between it and file_id.

richvdh · 2018-01-12T13:57:30Z

synapse/rest/media/v1/media_repository.py

+                                 server_name, media_id)
+                raise SynapseError(502, "Failed to fetch remote media")
+
+            yield finish()


yes, I realised that belatedly. sorry.

richvdh · 2018-01-12T14:17:49Z

synapse/rest/media/v1/media_storage.py

+                os.remove(fname)
+            except Exception:
+                pass
+            raise e


true. In that case please can youdo the fiddling with sys.exc_info (raise exc_info[0], exc_info[1], exc_info[2])?

erikjohnston · 2018-01-12T15:08:56Z

I've docced the media_id params, though whether they're useful is another question. I struggled a bit to come up with something better than "media_id is a media ID" :/ Suggestions welcome

richvdh

lgtm

erikjohnston · 2018-01-16T10:23:48Z

Thank you :)

erikjohnston force-pushed the erikj/media_storage_refactor branch from 44025b4 to d402d0f Compare January 9, 2018 16:14

erikjohnston added 7 commits January 9, 2018 16:15

Split out add_file_headers

47ca5eb

Add some helper classes

1ee7879

Add MediaStorage class

ada470b

Use MediaStorage for local files

dd3092c

Use MediaStorage for remote media

9e20840

Make ThumbnailResource use MediaStorage

9d30a76

Make PreviewUrlResource use MediaStorage

2442e98

erikjohnston force-pushed the erikj/media_storage_refactor branch from d402d0f to 6daa0d1 Compare January 9, 2018 16:15

Add StorageProvider concept

8f03aa9

erikjohnston force-pushed the erikj/media_storage_refactor branch from 6daa0d1 to 8f03aa9 Compare January 9, 2018 16:16

erikjohnston commented Jan 9, 2018

View reviewed changes

erikjohnston assigned richvdh Jan 9, 2018

richvdh suggested changes Jan 11, 2018

View reviewed changes

richvdh assigned erikjohnston and unassigned richvdh Jan 11, 2018

Comments

227c491

erikjohnston force-pushed the erikj/media_storage_refactor branch from 997a539 to 227c491 Compare January 12, 2018 11:22

erikjohnston added 5 commits January 12, 2018 11:23

Make class var local

4d88958

Remove unused variables

c6c0096

Remove unnecessary condition

1e4edd1

Merge branch 'develop' of github.com:matrix-org/synapse into erikj/me…

81391fa

…dia_storage_refactor

Add missing class var

dcc8ede

erikjohnston assigned richvdh and unassigned erikjohnston Jan 12, 2018

Make Responder a context manager

85a4d78

richvdh reviewed Jan 12, 2018

View reviewed changes

richvdh suggested changes Jan 12, 2018

View reviewed changes

richvdh assigned erikjohnston and unassigned richvdh Jan 12, 2018

erikjohnston added 2 commits January 12, 2018 14:44

Correctly reraise exception

e21370b

Fix up comments

694f1c1

erikjohnston assigned richvdh and unassigned erikjohnston Jan 12, 2018

erikjohnston mentioned this pull request Jan 15, 2018

Keep track of last access time for local media #2783

Merged

richvdh approved these changes Jan 15, 2018

View reviewed changes

richvdh assigned erikjohnston and unassigned richvdh Jan 15, 2018

erikjohnston merged commit 1159abb into develop Jan 16, 2018

erikjohnston mentioned this pull request Jan 16, 2018

Fix thumbnailing remote files #2789

Merged

neilisfragile mentioned this pull request May 9, 2018

Ability to store files in a CDN network or a S3 type server #1749

Closed

richvdh mentioned this pull request Aug 27, 2019

Remote dynamic thumbnails are stored at the wrong location #5913

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MediaRepository to separate out storage #2767

Refactor MediaRepository to separate out storage #2767

erikjohnston commented Jan 9, 2018

erikjohnston commented Jan 9, 2018

erikjohnston Jan 9, 2018

richvdh Jan 11, 2018

richvdh Jan 11, 2018

richvdh Jan 11, 2018

richvdh Jan 11, 2018

richvdh Jan 11, 2018

richvdh Jan 11, 2018

richvdh Jan 11, 2018

richvdh Jan 11, 2018

richvdh Jan 11, 2018

richvdh Jan 11, 2018

erikjohnston Jan 12, 2018

erikjohnston commented Jan 12, 2018

richvdh commented Jan 12, 2018

richvdh Jan 12, 2018

richvdh Jan 12, 2018

richvdh Jan 12, 2018

richvdh Jan 12, 2018

richvdh Jan 12, 2018

richvdh Jan 12, 2018

erikjohnston commented Jan 12, 2018

richvdh left a comment

erikjohnston commented Jan 16, 2018

		logger = logging.getLogger(__name__)


		class MediaStorage(object):

Refactor MediaRepository to separate out storage #2767

Refactor MediaRepository to separate out storage #2767

Conversation

erikjohnston commented Jan 9, 2018

erikjohnston commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikjohnston commented Jan 12, 2018

richvdh commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikjohnston commented Jan 12, 2018

richvdh left a comment

Choose a reason for hiding this comment

erikjohnston commented Jan 16, 2018