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.

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

Refactor \OC\Memcache\Factory #13368

Merged
merged 2 commits into from Mar 5, 2015

Conversation

Projects
None yet
9 participants
@Xenopathic
Copy link
Member

Xenopathic commented Jan 14, 2015

Caches divided up into two groups: distributed and local. 'Low latency' is an alias for local caches, while the standard create() call tries to get distributed caches first, then local caches.

New functions:

  • createLocal()/createDistributed() - creates local/distributed memcache
  • getAvailableLocal()/getAvailableDistributed() - gets an available backend class, or null

Fixes #13227

cc @LukasReschke @DeepDiver1975 @MorrisJobke

@MorrisJobke MorrisJobke added this to the 8.0-current milestone Jan 14, 2015

@MorrisJobke

This comment has been minimized.

Copy link
Member

MorrisJobke commented Jan 14, 2015

Works - tested with the app management and the speed gain is still measurable 👍

@Xenopathic

This comment has been minimized.

Copy link
Member Author

Xenopathic commented Jan 14, 2015

@MorrisJobke There's a speed gain? Wasn't expecting that - with one type of memcache enabled it the code should be identical to before, it's when multiple backends are available that it makes a difference. But still, performance is performance I guess!

@MorrisJobke

This comment has been minimized.

Copy link
Member

MorrisJobke commented Jan 14, 2015

@Xenopathic Yes and no. I just tested the speed gain, that is directly visible if there is a cache in place and detected ;) (to be exactly #13212)

@LukasReschke

View changes

lib/private/memcache/factory.php Outdated
* @param array $cacheList
* @return string|null
*/
private function getAvailable($cacheList) {

This comment has been minimized.

@LukasReschke

LukasReschke Jan 14, 2015

Member

Enforce the type?

@LukasReschke

View changes

lib/private/memcache/factory.php Outdated
}

/**
* get a in-server cache instance, will return null if no backend is available
* see createLocal()

This comment has been minimized.

@LukasReschke
@LukasReschke

View changes

lib/private/memcache/factory.php Outdated
}

/**
* check if there is a in-server backend available
* see getAvailableLocal()

This comment has been minimized.

@LukasReschke
@LukasReschke

View changes

lib/private/memcache/factory.php Outdated
}

/**
* get a in-server cache instance, will return null if no backend is available
* see createLocal()
*
* @param string $prefix
* @return null|Cache

This comment has been minimized.

@LukasReschke

LukasReschke Jan 14, 2015

Member

Will this actually ever return null?

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 14, 2015

from my understanding we need to expose these operations in the OCP namespace as well.
I'll come up with an api naming tomorrow - generally speaking the impl looks good - THX @Xenopathic

@LukasReschke

View changes

lib/private/memcache/factory.php Outdated
* create a distributed cache instance
*
* @param string $prefix
* @return \OC\Memcache\Cache

This comment has been minimized.

@LukasReschke

LukasReschke Jan 14, 2015

Member

May return void?

This comment has been minimized.

@Xenopathic

Xenopathic Jan 14, 2015

Author Member

void is not a type, and apparently using @return void is bad practise (PHP really returns null if there are no returns).

This comment has been minimized.

@LukasReschke

LukasReschke Jan 15, 2015

Member

Mhm… Anyways that PHPDoc is lieing that way. We need to adjust it.

This comment has been minimized.

@LukasReschke

LukasReschke Jan 15, 2015

Member

(currently static code analyzers have no chance to detect that you need to check the return type – thus we can't catch a lot of potential pitfalls)

@Xenopathic Xenopathic force-pushed the memcache_lowlatency branch Jan 14, 2015

@nickvergessen

View changes

lib/private/memcache/factory.php Outdated
private $localCaches = [
'\\OC\\Memcache\\APCu',
'\\OC\\Memcache\\APC',
'\\OC\\Memcache\\XCache'

This comment has been minimized.

@nickvergessen

nickvergessen Jan 15, 2015

Contributor

please trailing ,

@Xenopathic Xenopathic force-pushed the memcache_lowlatency branch Jan 15, 2015

@LukasReschke

View changes

lib/private/memcache/factory.php Outdated
@@ -17,6 +17,23 @@ class Factory implements ICacheFactory {
private $globalPrefix;

/**
* @var array $distributedCaches

This comment has been minimized.

@LukasReschke

LukasReschke Jan 15, 2015

Member

maybe use string[] ?

@LukasReschke

View changes

lib/private/memcache/factory.php Outdated
];

/**
* @var array $localCaches

This comment has been minimized.

@LukasReschke

LukasReschke Jan 15, 2015

Member

maybe use string[]?

@LukasReschke

View changes

lib/private/memcache/factory.php Outdated
* @param string $prefix
* @return null|Cache
* @return \OC\Memcache\Cache
*/
public function createLowLatency($prefix = '') {

This comment has been minimized.

@LukasReschke

LukasReschke Jan 15, 2015

Member

Do we still need this, I think createLocal does already take care of this? – I'd either remove the function completely and replace all occurrences or if it is widely used add a deprecation note?

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Jan 15, 2015

Member

Do we still need this, I think createLocal does already take care of this? – I'd either remove the function completely and replace all occurrences or if it is widely used add a deprecation note?

Depends on the pubic api I wanted to come up with ....

This comment has been minimized.

@Xenopathic

Xenopathic Jan 15, 2015

Author Member

@LukasReschke ah, good point, I forgot that this isn't in the public API so we can deprecate functions immediately 😄

@LukasReschke

View changes

lib/private/memcache/factory.php Outdated
}
}

/**
* get a cache instance, or Null backend if no backend available

This comment has been minimized.

@LukasReschke

LukasReschke Jan 15, 2015

Member

Add a note that this first uses the distributed caches and after that falls backs to local ones.

@LukasReschke

View changes

lib/private/memcache/factory.php Outdated
@@ -24,63 +41,99 @@ public function __construct($globalPrefix) {
}

/**
* @param array $cacheList
* @return string

This comment has been minimized.

@LukasReschke

LukasReschke Jan 15, 2015

Member

may theoretically return nothing as well? - add a return ''; at the end of the function?

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 15, 2015

Regarding API:
we basically have two options:

  • introduce a second method to ICacheFactory like
interface ICacheFactory{
    public function create($prefix = '');
    public function createLocal($prefix = '');
}
  • introduce a second parameter to the existing method like
interface ICacheFactory{
    public function create($prefix = '', $onlyLocal = false);
}

@LukasReschke @MorrisJobke @icewind1991 opinions?

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 15, 2015

The first one looks more logical to me as it may lead easily lead to confusion otherwise. For example in the remove case $onlyLocal with a default of false may imply that it deletes values from the distributed and the local cache.

Same is obviously valid for the get and other functions which effectively only will ask the distributed cache.

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 15, 2015

The first one looks more logical to me as it may lead easily lead to confusion otherwise. For example in the remove case $onlyLocal with a default of false may imply that it deletes values from the distributed and the local cache.

Same is obviously valid for the get and other functions which effectively only will ask the distributed cache.

@LukasReschke the cachfactors only has two methods: create and is available - remove and get are on the explicit cache implementations

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 15, 2015

the cachfactors only has two methods: create and is available - remove and get are on the explicit cache implementations

I know. Let try to put some code here so that my concern are more understandable :-)

When we take a look at the following snippet it is pretty much clear what it does, it clears the whole cache from values that have the prefix foo. Since there is no differentiation between the in-memory cache and distributed caches this will always clear all values.

\OC::$server->getMemCacheFactory()->create('foo')->clear();

With the proposed approach No. 2 we would now have:

// This will now clear all values from the remote memcaches only if one is available. 
// Though the parameter is called `onlyLocal` which is implicitly set to false in the interface
// so from a look at the interface once (at least me) would assume that this deletes values from
// the local AND the distributed cache (because `onlyLocal` false obviously does not exclude `local`
// itself) 
\OC::$server->getMemCacheFactory()->create('foo')->clear();

// This is actually what developers have to do to create the local memcache and not the distributed one
\OC::$server->getMemCacheFactory()->create('foo', true)->clear();

My take here is that this – at least for me – seems somewhat confusing and with two different functions it is actually more clean and verbose what happens here.

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 15, 2015

I see your point!

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 15, 2015

The code samples are not really valid - create() will always return a new instance and therefore a valid code snippet looks like this:

$cache = \OC::$server->getMemCacheFactory()->create('foo');
$cache->set(...);
$cache->get(...);
$cache->clear(...);
@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 15, 2015

Sure, but the key point stays there:

// Doing this will only clear the distributed cache
$cache = \OC::$server->getMemCacheFactory()->create('foo');
$cache->set(...);
$cache->get(...);
$cache->clear(...);
// Doing this will only clear the local cache
$cache = \OC::$server->getMemCacheFactory()->create('foo', true);
$cache->set(...);
$cache->get(...);
$cache->clear(...);

If we rename onlyLocal to useLocal the implementation would look cleaner to me. Then it is pretty much clear that to clean the local cache you need to pass true as well.

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 15, 2015

hmmm - I'm honestly not sure what the best approach might be - we also have to honor this flag in the isAvailable() method.

Let's add the public api in 8.1 - no need to rush here currently

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 15, 2015

For me $onlyLocal implies:

  • with a value of true: "I will only store data in the local cache and also only delete it from there"
    • it currently does exactly that
  • with a value of false:: "I will store data in whatever cache is there and delete from whatever cache has the data available."
    • it currently will store the data in whatever cache is there but will only delete from the memcache, so there may be an inconsistency when developers don't think about passing the parameter. (for values set with true)
@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 15, 2015

Care to create a ticket for that so we don't forget it? :-)

let's simply shift #13227 to 8.1

@nickvergessen

This comment has been minimized.

Copy link
Contributor

nickvergessen commented Feb 24, 2015

if none is set, use the array implementation?

@Xenopathic

This comment has been minimized.

Copy link
Member Author

Xenopathic commented Feb 24, 2015

Eww, the array implementation isn't a real memcache though 😆

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Feb 24, 2015

"Explicit cache configuration" means that if no cache is configured, no cache is used. No autodetection whatsoever.

@Xenopathic

This comment has been minimized.

Copy link
Member Author

Xenopathic commented Feb 24, 2015

@bantu OK, sounds reasonable. I take it the same applies if the requested cache is not available?

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Feb 24, 2015

@Xenopathic Good question. If the drivers for the configured cache is missing, we might want to throw a fatal error right away. Not sure what is supposed to happen when e.g. the memcached server is not available temporarily.

@Xenopathic Xenopathic force-pushed the memcache_lowlatency branch from e5cd891 to b1ab81e Feb 24, 2015

@Xenopathic

This comment has been minimized.

Copy link
Member Author

Xenopathic commented Feb 24, 2015

Right, memcache backend configuration is now done in config.php, with the keys memcache.local and memcache.distributed. memcache.distributed defaults to the value of memcache.local if not set. If the requested cache is unavailable (or null), the factory will return a Null backend for all create*() methods, however isAvailable() will still be strict and only return true if a non-Null backend is available.

Also, unit tests!

Please review @MorrisJobke @PVince81 @LukasReschke @DeepDiver1975

@Xenopathic Xenopathic changed the title [WIP] Refactor \OC\Memcache\Factory Refactor \OC\Memcache\Factory Feb 24, 2015

@icewind1991

This comment has been minimized.

Copy link
Member

icewind1991 commented Feb 25, 2015

Looks good 👍

@bantu bantu referenced this pull request Mar 2, 2015

Closed

memcached issues #6074

@Xenopathic Xenopathic force-pushed the memcache_lowlatency branch from b1ab81e to 8c6e4d6 Mar 4, 2015

@Xenopathic

This comment has been minimized.

Copy link
Member Author

Xenopathic commented Mar 4, 2015

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Mar 4, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10192/
Test PASSed.

* Memory caching backend configuration
*
* Available cache backends:
* - \OC\Memcache\APC Alernative PHP Cache backend

This comment has been minimized.

@LukasReschke

LukasReschke Mar 5, 2015

Member

Alternative

Robin McCorkell added some commits Jan 14, 2015

Robin McCorkell
Refactor \OC\Memcache\Factory
Caches divided up into two groups: distributed and local. 'Low latency' is an
alias for local caches, while the standard `create()` call tries to get
distributed caches first, then local caches.

Memcache backend is set in `config.php`, with the keys `memcache.local` and
`memcache.distributed`. If not set, `memcache.distributed` defaults to the value
of `memcache.local`.

@Xenopathic Xenopathic force-pushed the memcache_lowlatency branch from 8c6e4d6 to 78819da Mar 5, 2015

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Mar 5, 2015

As discussed in IRC: Needs some more work.

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Mar 5, 2015

The inspection completed: 7 new issues, 17 updated code elements

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Mar 5, 2015

Okay. – As discussed 👍 - needs documentation though…

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Mar 5, 2015

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Mar 5, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10223/
Test PASSed.

LukasReschke added a commit that referenced this pull request Mar 5, 2015

@LukasReschke LukasReschke merged commit 9f5433c into master Mar 5, 2015

1 check passed

default Merged build finished.
Details

@LukasReschke LukasReschke deleted the memcache_lowlatency branch Mar 5, 2015

Xenopathic pushed a commit to owncloud/documentation that referenced this pull request Mar 5, 2015

Xenopathic pushed a commit to owncloud/documentation that referenced this pull request Mar 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment