Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Drop file caching #16200

Merged
merged 5 commits into from
Jun 2, 2015
Merged

Drop file caching #16200

merged 5 commits into from
Jun 2, 2015

Conversation

blizzz
Copy link
Contributor

@blizzz blizzz commented May 8, 2015

File cache should be removed.

@jvillafanez this should also fix what you found in #16170 (comment)

cc @DeepDiver1975

@blizzz blizzz added this to the 8.1-current milestone May 8, 2015
@blizzz blizzz changed the title Kill globalfilecache Drop file caching May 8, 2015
@MorrisJobke
Copy link
Contributor 

00:16:07.886 There was 1 failure:
00:16:07.886 
00:16:07.886 1) Test\BackgroundJob\JobList::testGetNextWrapAround
00:16:07.886 null does not match expected type "object".
00:16:07.886 
00:16:07.886 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/tests/lib/backgroundjob/joblist.php:163

@blizzz
Copy link
Contributor Author

blizzz commented May 8, 2015

Ha, there is another possible usage https://github.com/owncloud/core/blob/master/lib/private/backgroundjob/joblist.php#L176

@blizzz
Copy link
Contributor Author

blizzz commented May 8, 2015

meh, i killed to much. UserCache is still in use.

hm, available as server service. killing it might break api, if in use.

@DeepDiver1975
Copy link
Member

Ha, there is another possible usage https://github.com/owncloud/core/blob/master/lib/private/backgroundjob/joblist.php#L176

we should simply return null in these two cases - these jobs are no longer required.
furthermore we need to have a repair job to kill the jobs as well

@DeepDiver1975
Copy link
Member

@blizzz any chance to get this going ? THX

@blizzz
Copy link
Contributor Author

blizzz commented May 12, 2015

@DeepDiver1975 here you go.

@blizzz
Copy link
Contributor Author

blizzz commented May 12, 2015

meh, oversaw another thing

@blizzz
Copy link
Contributor Author

blizzz commented May 13, 2015

Now we should be fine.

MorrisJobke
MorrisJobke reviewed May 13, 2015
View reviewed changes
lib/private/server.php
*
* @return \OCP\ICache
* @deprecated use getMemCacheFactory to obtain a proper cache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the version number of deprecation directly after the keyword:

@deprecated 8.1.0 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@scrutinizer-notifier
Copy link

A new inspection was created.

@blizzz blizzz mentioned this pull request May 13, 2015
Closed
@blizzz
Copy link
Contributor Author

blizzz commented May 13, 2015

Test Passed

DeepDiver1975
DeepDiver1975 reviewed May 19, 2015
View reviewed changes
lib/private/server.php
$this->registerService('UserCache', function ($c) {
return new UserCache();
$this->registerService('NullCache', function ($c) {
return new NullCache();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding we shall use ArrayCache here - otherwise potential users will get a different behavior.

objections @nickvergessen @MorrisJobke

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. ArrayCache will result in the better behaviour. Otherwise: The NullCache just doesn't cache anything and will "only" have impact on the performance.

@DeepDiver1975
Copy link
Member

👍 ready to go from my understanding

@icewind1991 @PVince81 @nickvergessen please review - THX

@MorrisJobke
Copy link
Contributor

👍 LDAP still works, the job got cleaned from the database, works

MorrisJobke added a commit that referenced this pull request Jun 2, 2015
@MorrisJobke
Merge pull request #16200 from owncloud/kill-globalfilecache
cf2c599 
Drop file caching
@MorrisJobke MorrisJobke merged commit cf2c599 into master Jun 2, 2015
@MorrisJobke MorrisJobke deleted the kill-globalfilecache branch June 2, 2015 07:11
This was referenced Jun 5, 2015
Closed
Closed
LukasReschke added a commit that referenced this pull request Jun 15, 2015
@LukasReschke
Reintroduce user cache
15ba2a4 
This was required by avatars and was broken with #16200

Fixes #16942
@LukasReschke LukasReschke mentioned this pull request Jun 15, 2015
Merged
@nickvergessen nickvergessen mentioned this pull request Jun 16, 2015
Closed
@MorrisJobke MorrisJobke mentioned this pull request Dec 8, 2015
Merged
@lock lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
3 - To Review
Projects
None yet
Milestone
8.1
Development

Successfully merging this pull request may close these issues.
5 participants
@blizzz @MorrisJobke @DeepDiver1975 @scrutinizer-notifier @nickvergessen