search

tollmanz / wordpress-pecl-memcached-object-cache

A WordPress object cache that uses the memcached (not memcache) PECL extension.
233 stars 114 forks source link

falling back to internal cache #54

Closed shmuel-krakower closed 9 years ago

shmuel-krakower commented 9 years ago

Seems like in case of no available memcached server, the behavior is to not have any caching. Instead - the behavior I suggest is to fallback to internal cache.

The outcome in tests I've made while shutting down all memcached nodes is that we get same performance as the original WP performance (i.e. like while not having this plugin at all). Without the suggested commit, we suffer from really bad performance due to not having any caching, which is really bad for WP.

shmuel-krakower commented 9 years ago

hi @tollmanz - not sure why those two tests are failing, I've manually tested them on my system and they work as expected. Before I setup the wordpress testing framework to run the phpunit tests locally, can you confirm that indeed the original behavior which I aim to fix, should be fixed?

shmuel-krakower commented 9 years ago

@tollmanz ?

tollmanz commented 9 years ago

@shmuel-krakower I did not see this PR come in. So sorry. I'll take a look soon

tollmanz commented 9 years ago

@shmuel-krakower it's choking on the CAS functions:

1) MemcachedUnitTestsGet::test_get_value_return_null_cas_token_with_not_found_key
Failed asserting that false is true.
/home/travis/build/tollmanz/wordpress-pecl-memcached-object-cache/tests/tests/get.php:257
2) MemcachedUnitTestsGet::test_get_by_key_value_return_null_cas_token_with_not_found_key
Failed asserting that false is true.
/home/travis/build/tollmanz/wordpress-pecl-memcached-object-cache/tests/tests/get.php:565

Obviously, we won't reproduce CAS functionality when the cache isn't working so we'll have to work around this with the tests. I still need to actually review the code, but if you want to start in on the tests, I would appreciate it.

shmuel-krakower commented 9 years ago

Hi @tollmanz - this was just the phpunit which was failing for me, unless I miss something else in the implementation, which I don't use.

I've tested it few times with some stress tests and it fixed the issues where I shut down memcached.

To be honest - this implementation works much better for me than another popular implementation - W3TC plugin which is based on the older Memcache php module. My tests show an actual improvement with loading times in compare to other implementations I've been looking into.

shmuel-krakower commented 9 years ago

Hi @tollmanz - I've finally found the time to setup and run the phpunit tests locally. By looking at it, it seems to break on the master branch as well, with the same errors as my PR.

Could you give it a try to run the tests locally on the master branch? Maybe I've missed something here with my setup.

It is possible that there are other changes on the external resources (from WP) which fail the tests.

tollmanz commented 9 years ago

Hi @shmuel-krakower! Thanks for following up. The failing tests are with regard to CAS functions. I get the same result as you. I cannot quite understand why they fail yet, but CAS operations are different than normal add/set operations. The CAS behaviors is not mimicked in the runtime/local cache so I would expect different results there.

We either need to:

  1. Change the patch to accommodate this scenario
  2. Update the tests to handle the odd situation

I like the general idea here. We just need to find a way to make it work with CAS operations. I'll take a look at some point, but don't anticipate having the time for a few weeks.

shmuel-krakower commented 9 years ago

Hi @tollmanz - maybe my last comment was not clear. Let me try to clarify. The CAS tests fail for me on the version which is on the master branch (not related to my PR at all). This means that unless I miss something on my local setup, that the external resources which the tests depend on, have been changed.

If you run the tests locally for the master branch, do the tests fail for you?

tollmanz commented 9 years ago

Got it. I did a little poking around and it seems I fixed this on the develop branch.

Can you try branching off of develop and making your changes? If you submit the PR, Travis will run the test automatically.

Sorry about the misunderstanding. I suspect that the PR will pass tests if branched off of develop.

shmuel-krakower commented 9 years ago

thanks for the quick replies, I'll give that a try tomorrow.

tollmanz commented 9 years ago

You deserve a few quick replies after waiting so long. I'm hoping to do a new release for this project in early summer. There are a lot of great improvements in the develop branch that I'd like to get out and I hope I can get this in there too.

shmuel-krakower commented 9 years ago

Hi @tollmanz - seems like branching develop works fine. I've created a new PR @ https://github.com/tollmanz/wordpress-pecl-memcached-object-cache/pull/55

Thanks