Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Cache] Simple Memcached Adapter#20858

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@robfrawley
Copy link
Contributor

@robfrawleyrobfrawley commentedDec 10, 2016•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PRsymfony/symfony-docs#7265
Related PRs#20863,#20752

@robfrawleyrobfrawley changed the titlesimple memcached cache pool adapter[Cache] Simple memcached cache pool adapterDec 10, 2016
@robfrawleyrobfrawley changed the title[Cache] Simple memcached cache pool adapter[Cache] Simple Memcached AdapterDec 10, 2016
@robfrawleyrobfrawleyforce-pushed thefeature-simple-memcached-cache-pool branch 4 times, most recently fromc5ed119 to88b9ab9CompareDecember 10, 2016 17:44
protected $skippedTests = array(
//'testExpiration' => 'Testing expiration slows down the test suite',
//'testHasItemReturnsFalseWhenDeferredItemIsExpired' => 'Testing expiration slows down the test suite',
//'testDefaultLifeTime' => 'Testing expiration slows down the test suite',
Copy link
ContributorAuthor

@robfrawleyrobfrawleyDec 10, 2016•
edited
Loading

Choose a reason for hiding this comment

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

These commented-out lines should be removed prior to merge.

@robfrawleyrobfrawleyforce-pushed thefeature-simple-memcached-cache-pool branch frome0d09f5 to740bce0CompareDecember 10, 2016 18:05

public static function setupBeforeClass()
{
if (!extension_loaded('memcached') || !version_compare(phpversion('memcached'), '2.1.0', '>')) {
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Only 2.2.x and 3.x are supported. Version 2.1.x causes symbol error in extension when requesting last result code.

Copy link
ContributorAuthor

@robfrawleyrobfrawleyDec 10, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Seehttps://travis-ci.org/symfony/symfony/jobs/182877309#L2410 for example of the above described issue. Only applicable tomemcached version2.1.0 in Travis PHP5.5 environment.

Choose a reason for hiding this comment

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

To the unaware reader (like me a few minutes ago), this reads as if 2.1.1 is fine. But there is no 2.1.x, x>0
Could the check be ">=2.2.0"?

Choose a reason for hiding this comment

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

btw, this check could be moved to apublic static function isSupported() on the adapter (see ApcuAdapter for similar concern)

robfrawley reacted with thumbs up emoji

private function getIdsByPrefix($namespace)
{
if (false === $ids = $this->client->getAllKeys()) {
Copy link
Member

@nicolas-grekasnicolas-grekasDec 11, 2016•
edited
Loading

Choose a reason for hiding this comment

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

I was wondering: how does this method play withMemcached::OPT_PREFIX_KEY? Does it ignore it (ie returnall keys, even those that don't start with the prefix) or does it filter by prefix?

Copy link
ContributorAuthor

@robfrawleyrobfrawleyDec 11, 2016•
edited
Loading

Choose a reason for hiding this comment

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

From the docs:

Memcached::getAllKeys() queries each memcache server and retrieves an array of all keys stored on them at that point in time [...]

So itsounds like it ignores that setting, but I haven't usedMemcached::OPT_PREFIX_KEY myself, so I don't think we'd find a conclusive answer without delving into the C code for the extension.

Copy link
ContributorAuthor

@robfrawleyrobfrawleyDec 11, 2016•
edited
Loading

Choose a reason for hiding this comment

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

FYI: Remember, I don't have a combination of libmemcached/ext-memcached that actually supports this method so testing this behavior is...difficult. I'll see if I can't compile a working combination using phpenv later tonight.

Choose a reason for hiding this comment

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

I think we should drop the use of getAllKeys and reduce doClear toreturn $this->client->flush();: you can't make it work, and it comes with many notices saying this can kill kittens.

*/
protected function doFetch(array $ids)
{
foreach ($this->client->getMulti($ids) as $id => $val) {
Copy link
Member

@nicolas-grekasnicolas-grekasDec 11, 2016•
edited
Loading

Choose a reason for hiding this comment

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

can't we justreturn $this->client->getMulti($ids);?

robfrawley reacted with thumbs up emoji
*/
protected function doHave($id)
{
return $this->client->get($id) !== false

Choose a reason for hiding this comment

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

sincefalse is a valid value, this can't work, isn't it?
I guess this should be

$this->client->get($id);return$this->client->getResultCode() === \Memcached::RES_SUCCESS;

robfrawley reacted with thumbs down emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

That is an error on my part: intended for that to be an OR (||). The idea is to avoid the additional call to$this->client->getResultCode() if the$this->client->get($id) isn'tfalse.

Copy link
ContributorAuthor

@robfrawleyrobfrawleyDec 11, 2016•
edited
Loading

Choose a reason for hiding this comment

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

This is what I meant to have there (and what I'd recommend we use):

protectedfunctiondoHave($id)    {return$this->client->get($id) !==false            ||$this->client->getResultCode() !== \Memcached::RES_NOTFOUND;    }

Kind of a micro-optimization, but worth avoiding an additional function call if unnecessary, IMHO.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Updated per above comment.

Choose a reason for hiding this comment

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

Checking against === RES_SUCCESS looks required to me, !== opens for all the other codes

Copy link
ContributorAuthor

@robfrawleyrobfrawleyDec 11, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Ah,that change does make sense. I missed the change from!== RES_NOTFOUND to=== RES_SUCCESS in your comment. Updated in latest push.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

We're almost good for the first step :)

use Symfony\Component\Cache\Adapter\MemcachedAdapter;

/**
* @author Rob Frawley 2nd <rmf@src.run>

Choose a reason for hiding this comment

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

we usualy don't add author on tests (even if test some have, but they could be removed)

robfrawley reacted with thumbs up emoji

public static function setupBeforeClass()
{
if (!extension_loaded('memcached') || !version_compare(phpversion('memcached'), '2.1.0', '>')) {

Choose a reason for hiding this comment

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

btw, this check could be moved to apublic static function isSupported() on the adapter (see ApcuAdapter for similar concern)

robfrawley reacted with thumbs up emoji
}

/**
* @group memcachedAdapter

Choose a reason for hiding this comment

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

we don't use groups like this

robfrawley reacted with thumbs up emoji
/**
* @group memcachedAdapter
*/
public function testAdapterAndClientCreationAndServers()

Choose a reason for hiding this comment

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

not sure this test is useful: it tests only test methods/behavior, I'd remove it

robfrawley reacted with thumbs up emoji
@robfrawleyrobfrawleyforce-pushed thefeature-simple-memcached-cache-pool branch from740bce0 to1143944CompareDecember 11, 2016 22:02
@robfrawley
Copy link
ContributorAuthor

robfrawley commentedDec 11, 2016•
edited
Loading

@nicolas-grekas New pushed code with changes per your comments (and in some cases, my responses).

Also, un-commented test skips at this point as prior tests have showntestExpiration,testHasItemReturnsFalseWhenDeferredItemIsExpired, andtestDefaultLifeTime pass and I assume we want to skip those once this code is merged.

Otherwise, a bit of cleanup in the test class as well. Should be good to go@nicolas-grekas! Let me know if I can do anything else to get this merge ready.

@robfrawleyrobfrawleyforce-pushed thefeature-simple-memcached-cache-pool branch 3 times, most recently from3b2676d to778d5e6CompareDecember 11, 2016 23:05
@robfrawley
Copy link
ContributorAuthor

robfrawley commentedDec 12, 2016•
edited
Loading

@nicolas-grekas I am unable to find any combination of libmemcached and ext-memcached that result in a non-false return for\Memcached::getAllKeys().

How do you feel about replacing this class'sgetIdsByPrefix() with the following:

privatefunctiongetIdsByPrefix($namespace){if (false === ($ids =$this->client->getAllKeys())     &&false === ($ids =$this->getAllKeysMemcache())) {returnfalse;    }returnarray_filter((array)$ids,function ($id)use ($namespace) {return0 ===strpos($id,$namespace);    });}privatefunctiongetAllKeysMemcache(){static$client =null;if (false ===$client        || (null ===$client            &&false ===$client =$this->createMemcacheClient())) {returnfalse;    }$ids =array();foreach ($client->getExtendedStats('slabs')as$group) {foreach ($groupas$slabId =>$metadata) {if (!is_array($metadata)) {continue;            }foreach ($client->getExtendedStats('cachedump', (int)$slabId,1000)as$slabIds) {if (is_array($slabIds)) {$ids =array_merge($ids,array_keys($slabIds));                }            }        }    }return$ids;}privatefunctioncreateMemcacheClient(){if (!extension_loaded('memcache')) {returnfalse;    }$client =new \Memcache();foreach ($this->client->getServerList()as$srv) {$client->addServer(array_shift($srv),array_shift($srv));    }return$client;}

This ensures the check for thememcache extension fallback for obtaining all keys only occurs once, and then allows for proper, namespace-enabled flushing if that extensionis available.... Thoughts?

Note: Mixing these extensions in terms oftheir values is dangerous, due to serialization differences, but in terms of simply returning keys is harmless.

Due to the fact that\Memcached::getAllKeys() seems perpetually broken on all major distributions/ext-versions I've tested, this seems like a possible functional (arguably hackie) solution...

Basic logic required for this implementation:

Whenext-memcacheIs Not Available

On First Call

  • Method call:getAllKeysMemcache()(final returnbool:false)
  • Conditional check:false === $client
  • Conditional check:null === $client
  • Conditional check:false === $client = $this->createMemcacheClient())
  • Conditional checkextension_loaded()

On Subsequent Calls

  • Method call:getAllKeysMemcache()(final returnbool:false)
  • Conditional check:false === $client

Whenext-memcacheIs Available

On First Call

  • Method call:getAllKeysMemcache()(final returnstring[])
  • Conditional check:false === $client
  • Conditional check:null === $client
  • Conditional check:false === $client = $this->createMemcacheClient())
  • Conditional checkextension_loaded()
  • Instantiate and add servers to\Memcache instance
  • Perform loop to get ids

On Subsequent Calls

  • Method call:getAllKeysMemcache()(final returnstring[])
  • Conditional check:false === $client
  • Perform loop to get ids

@nicolas-grekas
Copy link
Member

I think this is going too far, that we should give up and just call flush... :)

robfrawley reacted with thumbs up emoji

$this->client = $client;
}

public static function isSupported()

Choose a reason for hiding this comment

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

should be moved as first method in the class (same as e.g. ApcuAdapter)

@nicolas-grekasnicolas-grekas added this to the3.3 milestoneDec 12, 2016
@robfrawleyrobfrawleyforce-pushed thefeature-simple-memcached-cache-pool branch from778d5e6 to64f3cceCompareDecember 12, 2016 15:43
@robfrawley
Copy link
ContributorAuthor

robfrawley commentedDec 12, 2016•
edited
Loading

Ha; fair enough. This should be good to go in that case. Updated per most recent comments.

return $isCleared;
}

private function getIdsByPrefix($namespace)

Choose a reason for hiding this comment

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

ping@robfrawley: :)

protectedfunctiondoClear($namespace)    {return$this->client->flush();    }

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Added a commit withhttps://github.com/symfony/symfony/pull/20858/files#diff-e01499fab5ad7f5e36517d157bc13e2fR96 to check if any Travis environment actually has a functional combination of the extension and library. After checkinghttps://travis-ci.org/symfony/symfony/builds/183319461, if none actually utilize this, then I'm with you 100%.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Nope. None call the extra logic: removed.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Removing that also raises coverage to 100%, so it's a solid move in my book.

@robfrawleyrobfrawleyforce-pushed thefeature-simple-memcached-cache-pool branch from0673846 to94e8e88CompareDecember 12, 2016 17:44
@robfrawley
Copy link
ContributorAuthor

robfrawley commentedDec 12, 2016•
edited
Loading

@nicolas-grekas Should I make the samesetUp andtearDown changes@stof recommended for my other PR at#20863 (comment)

{
$toDelete = count($ids);
foreach ((array) $this->client->deleteMulti($ids) as $result) {
if (true === $result || \Memcached::RES_NOTFOUND === $result) {
Copy link
Member

@nicolas-grekasnicolas-grekasDec 12, 2016•
edited
Loading

Choose a reason for hiding this comment

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

so, only one remaining comment here: in which case can$result betrue? when the cast is in effect and only then (look at the doc, I'd say yes?)

Shouldn't the implem be this one?

if (true ===$results =$this->client->deleteMulti($ids)) {returntrue;}$toDelete =count($ids);foreach ($resultsas$result) {if (\Memcached::RES_SUCCESS ===$result || \Memcached::RES_NOTFOUND ===$result) {        --$toDelete;    }}return0 ===$toDelete;

Copy link
ContributorAuthor

@robfrawleyrobfrawleyDec 12, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Return of$this->client->deleteMulti($ids) is actually alwaysbool[] now that we haveisSupported() require>=2.2.0.

So the cast can be removed now, to simply have

$toDelete =count($ids);foreach ($this->client->deleteMulti($ids)as$result) {if (true ===$result || \Memcached::RES_NOTFOUND ===$result) {                --$toDelete;            }        }return0 ===$toDelete;

That was to support<2.2.0 extension versions that would return a singlebool

Copy link
ContributorAuthor

@robfrawleyrobfrawleyDec 12, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Scratch that, you're right. The check can now simply be:

$toDelete =count($ids);foreach ($this->client->deleteMulti($ids)as$result) {if (\Memcached::RES_SUCCESS ===$result || \Memcached::RES_NOTFOUND ===$result) {                --$toDelete;            }        }return0 ===$toDelete;

The situation where it returnsbool is behind us with the version requirement inisSupported().

@robfrawleyrobfrawleyforce-pushed thefeature-simple-memcached-cache-pool branch from94e8e88 to12de2aeCompareDecember 12, 2016 18:09
@nicolas-grekas
Copy link
Member

Let's go :)
👍

robfrawley reacted with hooray emoji

@fabpot
Copy link
Member

Thank you@robfrawley.

@fabpotfabpot merged commit12de2ae intosymfony:masterDec 13, 2016
fabpot added a commit that referenced this pull requestDec 13, 2016
This PR was merged into the 3.3-dev branch.Discussion----------[Cache] Simple Memcached Adapter| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        || Related PRs |#20863, ~~#20752~~Commits-------12de2ae memcached cache pool adapter
xabbuh added a commit to symfony/symfony-docs that referenced this pull requestMar 13, 2017
…ctoring (robfrawley, javiereguiluz)This PR was merged into the master branch.Discussion----------[Cache] Memcached Adapter Docs and Cache Pool Docs RefactoringDocumentation forsymfony/symfony#20863 andsymfony/symfony#20858. PRsymfony/symfony#20863 is still being written/finalized, so this is a WIP following that PR at this time.Resolves#7256.Commits-------7db929c corrected spelling/grammarc49d427 cleanup all cache adapter documentation, extend redis docse3eff5b Reviewed the entire articled4292b7 refactor cache pool documentation and add memcached adapter docs
@fabpotfabpot mentioned this pull requestMay 1, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

4 participants

@robfrawley@nicolas-grekas@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp