search

silverstripe / silverstripe-s3

Silverstripe module to store assets in S3 rather than on the local filesystem (SS4/SS5 only)
BSD 3-Clause "New" or "Revised" License
20 stars 25 forks source link

Review the use of content cache. #11

Closed sminnee closed 5 years ago

sminnee commented 7 years ago

Thanks to https://github.com/silverstripe/silverstripe-s3/pull/10 the S3 module now comes with a content cache.

This caches the content of assets so that we can generate a thumbnails without unnecessary extra requests to S3 during an upload.

This cache will have issues with disk space and consistency and is unlikely to be the best solution for this job. Ideally we should review and come up with a new approach.

sminnee commented 7 years ago

Mateusz had the following suggestion:

I think we should benchmark it with only a volatile in-request cache that is PSR-16 compliant. If it's good enough, provide only that, remove the persistent bits and the pre-warming interface. If it's not good enough, then provide the persistent thing as an option.

mateusz commented 7 years ago

The files are deleted from the content cache at the end of the request right now via register_shutdown_function, so that should be safe enough.

I'm not sure what is the default cache retention in the framework, but if the metadata cache persists between requests, that might cause problems with multi-node setups. Files might not appear to be deleted, or might not be renamed, stuff like that. A caveat in docs would be good, and defaulting to conservative within-request-caching-only even better (something like ArrayCache?).

For content cache, keeping the file data separate from the content cache can cause inconsistencies: warmFromPath caches the path without copying the file into the cache. The content cache should probably use a proprietary interface instead of PSR-16, so that it's clear it cannot really be replaced with standard components (it confused the hell out of me).

I think the conservative-by-default setting (i.e. in-request only) would be especially acceptable if the performance was ok with that (need to benchmark whatever is there to benchmark - uploads, asset page loads). And then documenting what's the impact of switching to opcache, or memcached, and how it plays with multi-node environments, so people can make an informed decision on how to shoot themselves in the foot :-)

obj63mc commented 5 years ago

Related to issues #16 #19 and #20 - not sure if this is need anymore now that all caching is handled by flysystem cache adapter.

madmatt commented 5 years ago

Happy to close this and we can review with the new changes in release 0.4.0.