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.

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

falling back to internal cache #54

Closed
wants to merge 3 commits into from

Conversation

shmuel-krakower
Copy link

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.

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 case.
@shmuel-krakower
Copy link
Author

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
Copy link
Author

@tollmanz ?

@tollmanz
Copy link
Owner

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

@tollmanz
Copy link
Owner

@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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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

@tollmanz
Copy link
Owner

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
Copy link
Author

Hi @tollmanz - seems like branching develop works fine.
I've created a new PR @ #55

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants