falling back to internal cache

[shmuel-krakower]

No description provided.

[shmuel-krakower]

This is following the discussion in PR #54

[shmuel-krakower]

Hi @tollmanz - any estimation as of when do you expect to review this PR ? I think it is pretty straight forward.

[tollmanz]

Hi @shmuel-krakower! Thanks again for the PR.

I am reviewing this now and I am afraid I do not understand the issue. I've read over the initial issue and have looked at the suggested code change and do not understand what the problem is. It seems that the logic was only slightly changed.

The unit tests pass all the same and the tests test each of the possible outcomes of the method. I am not sure what is different.

Can you try to articulate the problem again? If needed, feel free to show a code example that helps demonstrate the issue.

[shmuel-krakower]

Hi @tollmanz thanks for looking into it.
The thing is that if memcached server is down, wp in-memory caching is also skipped.
If you run my PR code, while memcached is down, with the following lines you will see the value is available by the wp_cache_get, while if you run it without my PR, it will not get you the value back.

$key   = 'dummy';
$value = '100';

wp_cache_add( $key, $value);

$dummy_value = wp_cache_get( $key );
echo "Value is " . $dummy_value . ".";

While you may think that this is ok that add fails while memcached is down, it is actually not ok, as WP doesn't try to failover by itself but expects the drop-in plugin to failover to internal memory caching. Without that behavior - having this plugin while memcached nodes are all down - results with no caching at all (including the default in-memory caching which is used everywhere in WP), which in turn show worse performance than WP original performance.

One important thing is that I just figured that I should implement the same for the set method and not only for add.

[tollmanz]

The thing is that if memcached server is down, wp in-memory caching is also skipped.

Ok...I think I follow now. In the add and set methods, values are only ever set to the internal cache if Memcached::RES_SUCCESS === $this->getResultCode() is true; however, this statement will never be true if the memcached server is down.

I'm on board with this; however, the change need is much more involved than the code here. I see that the following methods would be affected by this issue:

• add
• append
• cas
• decrement
• delete
• flush
• get
• getMulti
• increment
• prepend
• replace
• set
• setMulti

Basically, the code always verifies that the object was set to memcached before adding it to the internal cache so that during the current execution, the internal cache and external cache are in sync.

One approach would be to rethink this. I really like the idea that the internal and external caches are in sync and do not really like the idea of removing that code. Perhaps we could do something else, like detect if memcached is healthy before loading this module, allowing us to fallback to the internal implementation if it is not. Unfortunately, some of this finagling may cause some issues with how this cache interacts with core.

[shmuel-krakower]

Yes @tollmanz, you got it right and definitely this PR should include the modified behavior for all these mentioned methods, in case you accept it.

The main problem I have with the current implemntation is that once memcached is not available, we lose caching completely, including the internal/in-memory/per-request/default one. This is causing WP to respond very slowly because it is built in a way that it depends on having at least the default caching.

If you look at the wp source code (which I know you did) you see lots of core functions in which stuff is pulled from db and pushed into wp_cache_add/set... and later in the process it is getting the objects from the cache again. The current behavior makes the caches invalid completely in such a scenario and results with really bad behavior (which is worse than the default behavior where this plugin is not installed at all, because the default is in-memory which is always available).

I'll update the PR to include all the relevant code changes to the list of methods you mentioned before and you can decide if you wish to merge it or not. I agree that this may result with inconsistency between the interal cache and memcached but it should happen only during periods where memcached is unavailble anyway.

[shmuel-krakower]

Hi @tollmanz - I've just updated this PR with your suggestion to cover all methods to work with the internal cache even in case of memcached failure. Please note I didn't change get and getMulti - as their behavior is already aligned to query the internal cache first anyway.
Your unit tests on Travis have approved this change (although I will only test it myself tonight).

If you trust your tests - can you merge it?

[shmuel-krakower]

Just an update - my tests - both functional and performance have approved this this change to work as expected.

[tollmanz]

Hi @shmuel-krakower! Just wanted to acknowledge that I saw your message. Really buried right now and trying to get to this soon.

[shmuel-krakower]

@tollmanz - thanks for the note, I'll wait.
btw - my changes are already on our production environment on seems to work as expected.

[shmuel-krakower]

@tollmanz hi - quite some time since your last sign of life.
Any chance we can finalize this topic before end of this year? :)

btw - as the developer of this caching plugin you should be aware of this core WP bug:
https://core.trac.wordpress.org/ticket/33276#ticket

[danielbachhuber]

@shmuel-krakower I think this pull request will need tests that run the entire suite with memcached disabled — to verify the object does behave as expected when memcached has gone away.

[shmuel-krakower]

hmm that's good idea but I'm unsure how to do that.
It has been a while since I've run the phpunit tests for this plugin, but I'll take a look on Sunday and try to run them locally while memcached is not available.

The only question I have is how can I share this test results?
Maybe it would be better to change the tests running here with Travis CI - to run one additional test while memcached is not available?

[tollmanz]

hi - quite some time since your last sign of life.
Any chance we can finalize this topic before end of this year? :)

I assure you I am alive, just focused on other things at the moment. Potentially eying December as a time to work more on this plugin.

I think this pull request will need tests that run the entire suite with memcached disabled — to verify the object does behave as expected when memcached has gone away.

Great suggestion.

The only question I have is how can I share this test results?
Maybe it would be better to change the tests running here with Travis CI - to run one additional test while memcached is not available?

You can configure Travis to run with different environmental settings. For instance, you could set up an environment where memcached is not running. Unfortunately, it is hard to work out Travis CI integration locally. What I'd recommend doing is make a new branch off of this branch, change settings for the .travis.yml file, commit, push and PR, which will kick off a new build. Keep doing this until you get it right. It's clunky, but that's really the only way to do it.

[shmuel-krakower]

Hi @tollmanz could you give me a hint what's missing from my new PR - the travis tests cry on something missing at: #75

PHP Fatal error:  Class 'WP_REST_Server' not found in /tmp/wordpress-tests-lib/includes/spy-rest-server.php on line 3
PHP Stack trace:
PHP   1. {main}() /home/travis/.phpenv/versions/5.3.29/bin/phpunit:0
PHP   2. PHPUnit_TextUI_Command::main() /home/travis/.phpenv/versions/5.3.29/bin/phpunit:722
PHP   3. PHPUnit_TextUI_Command->run() phar:///home/travis/.phpenv/versions/5.3.29/bin/phpunit/phpunit/TextUI/Command.php:104
PHP   4. PHPUnit_TextUI_Command->handleArguments() phar:///home/travis/.phpenv/versions/5.3.29/bin/phpunit/phpunit/TextUI/Command.php:114
PHP   5. PHPUnit_TextUI_Command->handleBootstrap() phar:///home/travis/.phpenv/versions/5.3.29/bin/phpunit/phpunit/TextUI/Command.php:622
PHP   6. PHPUnit_Util_Fileloader::checkAndLoad() phar:///home/travis/.phpenv/versions/5.3.29/bin/phpunit/phpunit/TextUI/Command.php:792
PHP   7. PHPUnit_Util_Fileloader::load() phar:///home/travis/.phpenv/versions/5.3.29/bin/phpunit/phpunit/Util/Fileloader.php:42
PHP   8. include_once() phar:///home/travis/.phpenv/versions/5.3.29/bin/phpunit/phpunit/Util/Fileloader.php:58
PHP   9. require() /home/travis/build/tollmanz/wordpress-pecl-memcached-object-cache/tests/bootstrap.php:27
PHP  10. require() /tmp/wordpress-tests-lib/includes/bootstrap.php:101
Fatal error: Class 'WP_REST_Server' not found in /tmp/wordpress-tests-lib/includes/spy-rest-server.php on line 

[tollmanz]

@shmuel-krakower This sucks. Looks like there is a new dependency in the WP unit testing framework that is not satisfied here.

WP recently incorporated the REST API and this part of the unit test framework relies on it. I think we'll need to adjust the test set up altogether 😢

[shmuel-krakower]

Could we use/lock to an older test version which doesn't use the REST API?

[danielbachhuber]

#76 should have fixed the problem, but there's a deeper problem: the test suite is dependent on changes to the WordPress test suite that are only present in trunk. So it's not really possible now to run trunk tests against an older version of WordPress.

[shmuel-krakower]

@tollmanz please let me know what can I do the make this PR into the code base.
I don't want to stay hanging like this with maintaining local changes for much longer.

Maybe I could take something which will ease your effort?