Objectstore storage metadata layer handler by mrow4a · Pull Request #31486 · owncloud/core

mrow4a · 2018-05-20T12:47:47Z

Description

This PR implements metadata handler for objectstorage file objects. This is objectstore storage filecache handler, as in objectstore implementation filecache is storage source of truth.

Related Issue

Obsoletes Temporarily cache objectstore stat metadata for duration of storage->getMetadata #31482
This PR is much safer way of avoiding filecache hits then cache of filecache globally Cache in filecache #28166

Motivation and Context

Metastore handler uses single object entries caching. Objectstore metadata cache can be safely used since in objectstore, filecache is source of truth. However, to ensure high level of filecache -> storage cache consistency, only one object entry is being cached at the time, and any other call than ->get clears the cache. This sort of caching ensures that repetitive calls to filecache for the same object are consistent across application logic.

How Has This Been Tested?

Diagnostic app with new file puts

"master" branch with files_primary_s3 implementation

New & edit single file PUT

{"type":"SUMMARY","reqId":"65HpLqGkulVX9tsLkZoX","time":"2018-05-20T12:53:35+00:00","remoteAddr":"::1","user":"admin","method":"PUT","url":"/octest/remote.php/dav/files/admin/testnew.txt","diagnostics":{"totalSQLQueries":177,"totalSQLDurationmsec":20.37215232849121,"totalSQLParams":400,"totalEvents":16,"totalEventsDurationmsec":52.40273475646973}}

{"type":"SUMMARY","reqId":"4ShcmHORhPgT6GKWdHY5","time":"2018-05-20T12:53:35+00:00","remoteAddr":"::1","user":"admin","method":"PUT","url":"/octest/remote.php/dav/files/admin/testnew.txt","diagnostics":{"totalSQLQueries":177,"totalSQLDurationmsec":19.32239532470703,"totalSQLParams":407,"totalEvents":16,"totalEventsDurationmsec":63.67301940917969}}

"objectstore metalayer" branch with files_primary_s3 implementation

New & edit single file PUT

{"type":"SUMMARY","reqId":"yEkjg1qNcTeAbkr1r9Mu","time":"2018-05-20T12:52:38+00:00","remoteAddr":"::1","user":"admin","method":"PUT","url":"/octest/remote.php/dav/files/admin/testnew.txt","diagnostics":{"totalSQLQueries":136,"totalSQLDurationmsec":17.536163330078125,"totalSQLParams":318,"totalEvents":16,"totalEventsDurationmsec":44.013261795043945}}

{"type":"SUMMARY","reqId":"vGxuK4coHDR5KWz21wes","time":"2018-05-20T12:52:38+00:00","remoteAddr":"::1","user":"admin","method":"PUT","url":"/octest/remote.php/dav/files/admin/testnew.txt","diagnostics":{"totalSQLQueries":119,"totalSQLDurationmsec":16.158103942871094,"totalSQLParams":291,"totalEvents":16,"totalEventsDurationmsec":49.94559288024902}}

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

mrow4a · 2018-05-20T12:49:46Z

FYI @jvillafanez @butonic @DeepDiver1975 @PVince81 @patrickjahns

mrow4a · 2018-05-20T13:37:04Z

Hmm, @patrickjahns the only failing thing is php-fixer.

codecov · 2018-05-20T13:39:02Z

Codecov Report

Merging #31486 into master will increase coverage by 0.02%.
The diff coverage is 91.42%.

@@             Coverage Diff              @@
##             master   #31486      +/-   ##
============================================
+ Coverage     63.42%   63.45%   +0.02%     
- Complexity    18500    18530      +30     
============================================
  Files          1165     1166       +1     
  Lines         69386    69456      +70     
  Branches       1264     1264              
============================================
+ Hits          44006    44070      +64     
- Misses        25011    25017       +6     
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	`52.58% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`64.69% <91.42%> (+0.03%)`	`18530 <30> (+30)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...b/private/Files/ObjectStore/ObjectStoreStorage.php	`84.29% <100%> (+0.36%)`	`96 <3> (+3)`	⬆️
.../private/Files/ObjectStore/ObjectStoreMetadata.php	`90.62% <90.62%> (ø)`	`27 <27> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35eb867...8a499e0. Read the comment docs.

phil-davis · 2018-05-20T15:23:24Z

+	public function searchByMime($mimetype) {
+		$this->metaDataCache->clear();
+		return parent::searchByMime($mimetype);
+


php-cs-fixer is complaining about this bonus empty line.

jvillafanez · 2018-05-21T06:49:41Z

+		// only one object entry is being cached at the time.
+		// This sort of caching ensures that repetitive calls to filecache
+		// for the same object are consistent across application logic
+		if ($this->metaDataCache === null) {


It seems a useless check

Hm, yes, thanks!

jvillafanez · 2018-05-21T06:58:04Z

+		// This sort of caching ensures that repetitive calls to filecache
+		// for the same object are consistent across application logic
+		if ($this->metaDataCache === null) {
+			$this->metaDataCache = new CappedMemoryCache(1);


I have mixed feelings using a CappedMemoryCache like this. It's more efficient to use just a plain variable if the plan is to store just one key.
I'd rather use a parameter for the CappedMemoryCache so if we decide to increase the cache size we can do that easily.

My complain here is that you're misusing the CappedMemoryCache object.

Ye, to be honest I left it with CappedMemoryCache just to keep a discussion on whether other files shall be cached too.

But generally yes, it could be variable

jvillafanez · 2018-05-21T07:02:35Z

+		$key = $this->getNumericStorageId().'-get-'.$file;
+		if ($this->metaDataCache->hasKey($key)) {
+			$metadata = $this->metaDataCache->get($key);
+			return new CacheEntry($metadata);


Why don't we store the CacheEntry in order to avoid creating a new object here?

To make full clone of the object, just to be sure nothing will modify that cache elsewhere

I think it will perform just a shallow clone. In case we have references to objects in the $metadata, those references will be kept.
If a shallow clone is fine, I'd use clone. In any case, we should document that this method returns a shallow clone of the metadata. This should help both to know that you aren't expected to modify the data (or that the changes won't affect to other objects), as well as that you should be careful with references inside the metadata, if any.

jvillafanez · 2018-05-21T07:09:43Z

+	 * @inheritdoc
+	 */
+	public function getId($file) {
+		$this->metaDataCache->clear();


This feels kind of harsh... Is it possible to use the cached data? I'd prefer to add 1 or 2 private methods to fetch the data from the cache and then from the DB to increase the reusability. (The same for all the methods)

The idea was to only cache repetitive calls to the same "cache function e.g. get". Thus any other call should clear cache. But yes, generally all "getid" could use cache and not clear.

jvillafanez · 2018-05-21T07:41:04Z

+			$storage = $this;
+		}
+		if ($storage && !isset($storage->cache)) {
+			$storage->cache = new ObjectStoreMetadata($storage);


I know you've just copy & paste the code, but this is awful. You can potentially set an ObjectStoreMetadata to any storage from this object, and you can end up with a "normal" cache here, which makes absolutely no sense.

@DeepDiver1975 Any chance to deprecate / remove this API and provide a better one? I think the easiest and least intrusive change is changing the implementation as shown below, but there is a cyclic dependency which is quite bad. (The same for the getScanner, getWatcher, and related methods)

public function getCache($path = '', $storage = null) { if ($storage === null) { $this->cache = new ObjectStoreMetadata($this); return $this->cache; } else { return $storage->getCache($path); } }

Yes, this was quite confusing for me too.

DeepDiver1975 · 2018-06-22T14:37:27Z

@ownclouders rebase

ownclouders · 2018-06-22T14:37:31Z

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

DeepDiver1975 · 2018-06-22T14:38:04Z

throwing this into smashbox would be great @mrow4a can you take care?
please post results - THX

ownclouders · 2018-06-22T14:40:31Z

Automated rebase with GitMate.io was successful! 🎉

mrow4a · 2018-06-22T16:20:25Z

Yes, will do! But also needs an opinion of Jorn @butonic on that (design)

mrow4a · 2018-06-22T16:30:22Z

@butonic @DeepDiver1975 please mind that it makes a lot of sense for Objectstore (only?), since source of truth for objectstorage is oc_filecache itself (thus it is very hard to mess sth up on storage level and not having it reflected in oc_filecache, while with filesystem I can easily do that)

butonic · 2018-06-27T21:13:12Z

I understand that you are caching only repeated gets for the same entry and invalidate them whenever anything changes. I still wonder why all the objectstore suite is green, when several tests change oc_filecache without telling your ObjectStoreMetadata.php, eg in https://github.com/owncloud/core/pull/28166/files#diff-052d4c915b07e1eae830a2aeaa62cb5cL103 https://github.com/owncloud/core/pull/28166/files#diff-052d4c915b07e1eae830a2aeaa62cb5cL152 https://github.com/owncloud/core/pull/28166/files#diff-052d4c915b07e1eae830a2aeaa62cb5cL203 https://github.com/owncloud/core/pull/28166/files#diff-052d4c915b07e1eae830a2aeaa62cb5cL252 or all the other places in that PR ...

Personally, I would prefer having this cache inside Cache, so all implementations can benefit from the caching. But again, I am currently puzzled why the tests don't fail. I will need to take a closer look.

mrow4a · 2018-06-27T21:21:44Z

I think because of public function getCache($path = '', $storage = null), in some cases when the cache class is already set, you use native FileCache, and not metadata.

EDIT: hmm actually now I am not sure.

mrow4a · 2018-06-30T10:19:14Z

This needs to be blocked until decision is made on temporarily caching filecache

mrow4a · 2018-06-30T17:22:53Z

Due to the fact there is no decision on introducing "cache of filecache", the PR #31958 is just fixing the code logic.

This was referenced May 20, 2018

Temporarily cache objectstore stat metadata for duration of storage->getMetadata #31482

Closed

Cache in filecache #28166

Closed

mrow4a mentioned this pull request May 20, 2018

Don't check for existence twice if we know it is not a part file (objectstorage only) #31299

Closed

1 task

mrow4a force-pushed the obj_cache branch from b5d9b57 to e763c8e Compare May 20, 2018 13:38

phil-davis reviewed May 20, 2018

View reviewed changes

jvillafanez reviewed May 21, 2018

View reviewed changes

ownclouders added the status/STALE label May 30, 2018

mrow4a force-pushed the obj_cache branch from e763c8e to 3e86207 Compare June 13, 2018 21:03

ownclouders removed the status/STALE label Jun 13, 2018

mrow4a force-pushed the obj_cache branch 3 times, most recently from 7764238 to d861184 Compare June 17, 2018 08:46

DeepDiver1975 self-assigned this Jun 22, 2018

DeepDiver1975 added the 3 - To Review label Jun 22, 2018

DeepDiver1975 added this to the development milestone Jun 22, 2018

ownclouders force-pushed the obj_cache branch from d861184 to 6931ffa Compare June 22, 2018 14:39

Objectstore storage metadata layer handler

8a499e0

mrow4a force-pushed the obj_cache branch from 6931ffa to 8a499e0 Compare June 25, 2018 18:32

mrow4a mentioned this pull request Jun 30, 2018

[objectstorage] Cache stat while fetching objectstore object metadata #31958

Merged

mrow4a closed this Jun 30, 2018

PVince81 deleted the obj_cache branch September 27, 2018 13:33

lock Bot locked as resolved and limited conversation to collaborators Sep 27, 2019

Uh oh!

Conversation

mrow4a commented May 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

"master" branch with files_primary_s3 implementation

"objectstore metalayer" branch with files_primary_s3 implementation

Types of changes

Checklist:

Uh oh!

mrow4a commented May 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrow4a commented May 20, 2018

Uh oh!

codecov Bot commented May 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 commented Jun 22, 2018

Uh oh!

ownclouders commented Jun 22, 2018

Uh oh!

DeepDiver1975 commented Jun 22, 2018

Uh oh!

ownclouders commented Jun 22, 2018

Uh oh!

mrow4a commented Jun 22, 2018

Uh oh!

mrow4a commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

butonic commented Jun 27, 2018

Uh oh!

mrow4a commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrow4a commented Jun 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrow4a commented Jun 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mrow4a commented May 20, 2018 •

edited

Loading

mrow4a commented May 20, 2018 •

edited

Loading

codecov Bot commented May 20, 2018 •

edited

Loading

mrow4a commented Jun 22, 2018 •

edited

Loading

mrow4a commented Jun 27, 2018 •

edited

Loading

mrow4a commented Jun 30, 2018 •

edited

Loading