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

Fixed non-cache stuff for memcached #133

Closed
wants to merge 1 commit into from

Conversation

007
Copy link

@007 007 commented Jun 18, 2019

This allows you to specify any cache. If a redis-type cache with a
pipeline is used, you get pipeline support. If not, the cache will still
be used, but without atomic transactions.

This allows you to specify any cache. If a redis-type cache with a
pipeline is used, you get pipeline support. If not, the cache will still
be used, but without atomic transactions.
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.3%) to 87.629% when pulling cc6ae8c on CodeMonk:release/cache-fix into 2a5527a on kencochrane:master.

@kencochrane
Copy link
Collaborator

@007 Thanks for the PR, a few questions.

  1. Have you tested this with memcached and does it work as it should?
  2. Which Cache Backends would work for this, and which ones wouldn't
  3. Do you know if there are any mock memcached libraries that could be used to write tests for memcached? The code coverage went down with the PR and I assume is it because of the lack of tests around the new code, which hopefully we can fix with some more tests.
  4. We should also update the docs if this changes how things work and include new functionality.

@007
Copy link
Author

007 commented Jul 4, 2019

@CodeMonk could speak to it better than I can, but I'll give it a go.

  1. Yes and yes. He implemented it for memcached because that's what we have for our external cache. We're using it in production and have tested that it does what it says on the tin.

  2. From my limited understanding it should work with any CACHE backend that supports get, set, incr, add, touch and keys, with support for pipeline if it exists.

  3. mockcache at least, but per (2) it should work with any cache that supports those semantics.

  4. Agreed, but I didn't fork and update because this module was 100% redis and I didn't know how you'd feel about ALL THE CACHES versus supporting redis and maybe-memcached.

@kencochrane
Copy link
Collaborator

@CodeMonk @007 I'm fine with supporting different backends, as long as they work as planned and are fast, and we have tests to prove they work now, and won't break in the future if something changes.

I didn't know that Memcached supports the touch, keys commands, is that a new feature?

Any chance you could add some tests mockcache tests?

@CodeMonk
Copy link

CodeMonk commented Jul 9, 2019

@kencochrane Memcached does NOT support keys, but, I believe it supports touch (by updating the expiry)

Far as I know, key listing and pipeline are the only unsupported features between memcached and redis.

I'll start working on some tests . . .

@kencochrane
Copy link
Collaborator

Where are we with this PR, if we are no longer going to make the change let’s close it.

If this is a feature people want, we will need someone to take over and make the changes needed to get it merged. Any volunteers?

@CodeMonk
Copy link

Sorry - a huge change came in before my PR was approved.

I'm running live with the code from my PR, but, I haven't had a chance to refactor for the changes upstream.

I'll close the PR for now.

@CodeMonk
Copy link

Or you can close - I may not have perms.

@007 007 closed this Jul 17, 2020
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.

None yet

4 participants