New Cacheable Flysystem Adapter #29077

Hemant-Mann · 2017-09-21T21:04:08Z

Description

I have added a new cacheable flysystem adapter in owncloud so that we can connect flysystem adapters and owncloud storage

Related Issue

None

Motivation and Context

This change is required because current flysystem implementation does not utilize caching and according to owncloud documentation this should be a must feature

Within a single PHP request, ownCloud might call the same storage methods repeatedly, due to different checks which it needs to carry out. As a result, there is the potential to incur significant overhead, when working with the underlying filesystem

How Has This Been Tested?

This has been tested with the new Dropbox App in which instead of extending the storage OCP\Files\Storage\FlysystemStorageAdapter I now extend it with OCP\Files\Storage\CacheableFlysystemAdapter

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

phil-davis · 2017-09-22T02:55:44Z

Note: the Travis-CI UI test jobs that make use of SauceLabs fail because they do not have any SauceLabs credentials. This is because the PR has come from a forked repo. Only PRs coming from the "master" ownCloud core repo have access to the ownCloud SauceLabs credentials, which are stored securely in the ownCloud Travis-CI account.

tomneedham · 2017-10-03T10:56:50Z

lib/private/Files/Storage/CacheableFlysystem.php

+				$this->cacheContents[$location] = $this->flysystem->getMetadata($location);
+			} catch (FileNotFoundException $e) {
+				// do not store this info in cache as it might interfere with Upload process
+                return false;


indentation

CLAassistant · 2017-10-09T18:36:43Z

All committers have signed the CLA.

PVince81 · 2017-10-10T14:58:54Z

lib/private/Files/Storage/CacheableFlysystem.php

+
+	public function buildPath($path) {
+		$location = parent::buildPath($path);
+		if ($this->isCaseInsensitiveStorage) {


ewww, is that a thing in Dropbox ?!

Yes unfortunately it is
Suppose you have the following directory structure

Foo/ ├── bar │ ├── foo.txt │ └── some_another_file.txt └── somefile.txt

In one case dropbox api was returning the folder name Foo as both Foo and foo due to which the directory list of the folder Foo was coming as empty but in reality it was not

PVince81 · 2017-10-10T14:59:31Z

lib/private/Files/Storage/CacheableFlysystem.php

+	 * Check if Cache Contains the data for given path, if not then get the data
+	 * from flysystem and store it in cache for this request lifetime
+	 *
+	 * @param  string $path Path to file/folder


document $overRideCache

done see below this line

PVince81

See comments above, more to come

PVince81 · 2017-10-10T15:00:55Z

lib/private/Files/Storage/CacheableFlysystem.php

+			try {
+				$this->cacheContents[$location] = $this->flysystem->getMetadata($location);
+			} catch (FileNotFoundException $e) {
+				// do not store this info in cache as it might interfere with Upload process


We should look into storing this info at some point. I know it works because we do this for other storages like DAV.

Maybe the upload interference is a missing stat clear in another place in the code.

I suggest to look into this separately.

Yes i faced some problem with upload process if i stored the info that file is not present in storage. i don't remember exactly what it was

PVince81 · 2017-10-10T15:03:28Z

lib/public/Files/Storage/CacheableFlysystemAdapter.php

+ * Public Cacheable storage adapter to be extended to connect to
+ * flysystem adapters
+ *
+ * @since 10.0.x


make that 10.0.4

PVince81 · 2017-10-10T15:06:13Z

lib/public/Files/Storage/CacheableFlysystemAdapter.php

+	 * @return string storage id
+	 * @since 10.0
+	 */
+	abstract public function getId();


I don't think you should redefined this.

Please make the CacheableFlysystemAdpater extend directly from FlysystemStorageAdapter which already provides this method.

PVince81 · 2017-10-10T15:08:24Z

Is this mtime caching specific to Dropbox ? https://github.com/owncloud/files_external_dropbox/blob/master/lib/Storage/Dropbox.php#L71

Isn't this information already available in this cache ? If not, does it make sense to bring this into your cacheable adapter as well or is it very specific to Dropbox ?

PVince81 · 2017-10-10T15:09:40Z

Shouldn't you cache the results you got here https://github.com/owncloud/files_external_dropbox/blob/master/lib/Storage/Dropbox.php#L144 into the local cache as well ?

PVince81 · 2017-10-10T15:10:45Z

Seems isCaseInsensitiveStorage is unused, remove ? https://github.com/owncloud/files_external_dropbox/blob/master/lib/Storage/Dropbox.php#L58

PVince81 · 2017-10-10T15:23:21Z

lib/private/Files/Storage/CacheableFlysystem.php

+
+			default:
+				unset($this->cacheContents[$this->getCacheLocation($path)]);
+				return parent::fopen($path, $mode);


it should be possible to invalidate the cache after an upload, see how it's done here: https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/DAV.php#L405 and https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/DAV.php#L503

it should be possible to invalidate the cache after an upload, see how it's done here: https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/DAV.php#L405

But then I'll have to modify the original Flysystem.php which does not have any concept of cache
OR
i'll have to duplicate the fopen function here and then add cache clear line after upload

I added it before to prevent copy/paste of duplicate code and even if file upload failed the cache is only set for a single request so I did not think it would be an issue

Hemant-Mann · 2017-10-10T16:18:23Z

Is this mtime caching specific to Dropbox ? https://github.com/owncloud/files_external_dropbox/blob/master/lib/Storage/Dropbox.php#L71

Isn't this information already available in this cache ? If not, does it make sense to bring this into your cacheable adapter as well or is it very specific to Dropbox ?

Yes it is specific to dropbox because in API v2 we don't get the modified time of folders so what I did was find the modified time of the files in the folder and get the largest of those values and this info is not available in cache

Hemant-Mann · 2017-10-10T16:21:05Z

Shouldn't you cache the results you got here https://github.com/owncloud/files_external_dropbox/blob/master/lib/Storage/Dropbox.php#L144 into the local cache as well ?

Uhh I am only fetching this info if not found in cache here https://github.com/owncloud/files_external_dropbox/blob/master/lib/Storage/Dropbox.php#L139

because the cache is only for a single request cycle i think this should work

Hemant-Mann · 2017-10-10T16:24:02Z

Seems isCaseInsensitiveStorage is unused, remove ? https://github.com/owncloud/files_external_dropbox/blob/master/lib/Storage/Dropbox.php#L58

Yes it is unused in Storage/Dropbox.php so we can remove it
but it is used here -https://github.com/owncloud/files_external_dropbox/blob/master/lib/Storage/CacheableFlysystemAdapter.php#L69

Where you asked

ewww, is that a thing in Dropbox ?!

PVince81

👍 fine for now, will need further optimizations in the future by adding the cache stuff in more code paths (fopen, etc)

PVince81 · 2017-11-02T09:00:55Z

@Hemant-Mann can you rebase ? Then do another PR to backport to stable10.

Hemant-Mann · 2017-11-02T11:23:10Z

Okay I'll do that

Hemant-Mann · 2017-11-02T14:36:06Z

Stable 10 PR #29414

👍 fine for now, will need further optimizations in the future by adding the cache stuff in more code paths (fopen, etc)

Yes we can definitely add cache stuff at more places like fopen

PVince81 · 2017-11-02T14:49:21Z

@Hemant-Mann thanks for the backport, can you also rebase this PR to kick CI ?

phil-davis · 2017-11-02T15:27:23Z

Note: this PR is coming from a forked repo, so the Travis-Saucelabs UI tests will fail due to Saucelabs authentication issues.

Hemant-Mann · 2017-11-02T16:22:03Z

@PVince81 Can you explain a little bit as to why I have to rebase it?

Is it related to @phil-davis issue

Note: this PR is coming from a forked repo, so the Travis-Saucelabs UI tests will fail due to Saucelabs authentication issues.

phil-davis · 2017-11-02T16:30:31Z

I see it also did not trigger the drone CI. There were some times recently when drone was down, so the job did not trigger. So rebasing will make the drone CI run.
(It will make all the other CI run also, and Travis will fail for the reason above, which is fine since we understand why)

Hemant-Mann · 2017-11-02T16:34:36Z

I rebased the branch with master but while pushing it rejected the commits and then I had to do a git pull which resulted in a merge commit

commit 5b1a17b6df9316d02f01d095ca64afc8a8869f60 (HEAD -> cacheable_storage_adapter)
Merge: b066f40340 fb6e3978b9
Author: Hemant Mann <hemant.mann121@gmail.com>
Date:   Thu Nov 2 22:01:50 2017 +0530

    Merge branch 'cacheable_storage_adapter' of https://github.com/Hemant-Mann/owncloud_core into cacheable_storage_adapter

So should I push this rebased branch?

phil-davis · 2017-11-02T16:39:43Z

You can locally do an interactive rebase:

git rebase -i master

and squash the commits together in whatever way seems logical (squashing out the merge commit...)
then force push:

git push -f

that will replace whatever is here in the PR branch with whatever you have done locally.

Hemant-Mann · 2017-11-02T16:42:20Z

Thanks I was just missing the -f flag in push that's why it was rejecting it

lock · 2019-08-02T02:03:03Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

PVince81 mentioned this pull request Sep 23, 2017

Release plan for first version owncloud/files_external_dropbox#2

Closed

12 tasks

pmaier1 added this to the development milestone Sep 28, 2017

pmaier1 requested a review from PVince81 September 28, 2017 09:19

pmaier1 added the p2-high Escalation, on top of current planning, release blocker label Sep 28, 2017

pmaier1 mentioned this pull request Oct 2, 2017

[For 10.1] Dropbox External Storage Release 1.1.0 owncloud/files_external_dropbox#10

Closed

31 tasks

tomneedham reviewed Oct 3, 2017

View reviewed changes

Hemant-Mann force-pushed the cacheable_storage_adapter branch from b3c9abd to 5a5263c Compare October 9, 2017 18:40

PVince81 reviewed Oct 10, 2017

View reviewed changes

PVince81 approved these changes Nov 2, 2017

View reviewed changes

PVince81 added the 4 - To release label Nov 2, 2017

Hemant-Mann mentioned this pull request Nov 2, 2017

[Backport] Stable10 cacheable storage adapter #29414

Merged

9 tasks

Hemant-Mann added 4 commits November 2, 2017 22:11

New Cacheable Flysystem Adapter

c2502e6

Fix some edge cases when using cache results

0387702

Fix indentation

ffba410

Update Requested Changes

3f57003

Hemant-Mann force-pushed the cacheable_storage_adapter branch from fb6e397 to 3f57003 Compare November 2, 2017 16:42

PVince81 merged commit 817d730 into owncloud:master Nov 3, 2017

ownclouders added the status/STALE label Dec 20, 2017

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Cacheable Flysystem Adapter #29077

New Cacheable Flysystem Adapter #29077

Hemant-Mann commented Sep 21, 2017 •

edited

Loading

phil-davis commented Sep 22, 2017

tomneedham Oct 3, 2017

CLAassistant commented Oct 9, 2017 •

edited

Loading

PVince81 Oct 10, 2017

Hemant-Mann Oct 10, 2017

PVince81 Oct 10, 2017

Hemant-Mann Oct 10, 2017

PVince81 left a comment

PVince81 Oct 10, 2017

Hemant-Mann Oct 10, 2017 •

edited

Loading

PVince81 Oct 10, 2017

PVince81 Oct 10, 2017

PVince81 commented Oct 10, 2017

PVince81 commented Oct 10, 2017

PVince81 commented Oct 10, 2017

PVince81 Oct 10, 2017

Hemant-Mann Oct 10, 2017 •

edited

Loading

Hemant-Mann commented Oct 10, 2017

Hemant-Mann commented Oct 10, 2017

Hemant-Mann commented Oct 10, 2017

PVince81 left a comment

PVince81 commented Nov 2, 2017

Hemant-Mann commented Nov 2, 2017

Hemant-Mann commented Nov 2, 2017

PVince81 commented Nov 2, 2017

phil-davis commented Nov 2, 2017 •

edited

Loading

Hemant-Mann commented Nov 2, 2017 •

edited

Loading

phil-davis commented Nov 2, 2017

Hemant-Mann commented Nov 2, 2017

phil-davis commented Nov 2, 2017

Hemant-Mann commented Nov 2, 2017 •

edited

Loading

lock bot commented Aug 2, 2019

New Cacheable Flysystem Adapter #29077

New Cacheable Flysystem Adapter #29077

Conversation

Hemant-Mann commented Sep 21, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

phil-davis commented Sep 22, 2017

Choose a reason for hiding this comment

CLAassistant commented Oct 9, 2017 • edited Loading

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

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hemant-Mann Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Oct 10, 2017

PVince81 commented Oct 10, 2017

PVince81 commented Oct 10, 2017

Choose a reason for hiding this comment

Hemant-Mann Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Hemant-Mann commented Oct 10, 2017

Hemant-Mann commented Oct 10, 2017

Hemant-Mann commented Oct 10, 2017

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Nov 2, 2017

Hemant-Mann commented Nov 2, 2017

Hemant-Mann commented Nov 2, 2017

PVince81 commented Nov 2, 2017

phil-davis commented Nov 2, 2017 • edited Loading

Hemant-Mann commented Nov 2, 2017 • edited Loading

phil-davis commented Nov 2, 2017

Hemant-Mann commented Nov 2, 2017

phil-davis commented Nov 2, 2017

Hemant-Mann commented Nov 2, 2017 • edited Loading

lock bot commented Aug 2, 2019

Hemant-Mann commented Sep 21, 2017 •

edited

Loading

CLAassistant commented Oct 9, 2017 •

edited

Loading

Hemant-Mann Oct 10, 2017 •

edited

Loading

Hemant-Mann Oct 10, 2017 •

edited

Loading

phil-davis commented Nov 2, 2017 •

edited

Loading

Hemant-Mann commented Nov 2, 2017 •

edited

Loading

Hemant-Mann commented Nov 2, 2017 •

edited

Loading