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

Modify cache redis backend #11608

Closed
wants to merge 2 commits into
base: 2.1.x
from

Conversation

Projects
None yet
9 participants
@pantaovay
Copy link

pantaovay commented Mar 30, 2016

Changes

  1. Modify cache redis backend, delete _PHCR
  2. Cache redis backend supports "client" option, now you can pass a redis client
  3. Redis's set method supports ttl,the call of settimeout is removed
  4. Redis has existsmethod to check if a key exists, the previous implementation uses get which is wrong

Why

  1. The redis cache backend use a special key _PHCR to store all keys used by cache. There is a issue: #10905 .According to @Green-Cat ,it is disabled in 2.0.x branch which is not true. If use _PHCR, every time we set a cache, we have to run sAdd and set commands. Store all keys in _PHCR will cause it expands quickly and this will influence performance.
  2. The previous implementation mixes statsKey and prefix, it uses _PHCR as super prefix which is not needed.
  3. The cache backend doesn't support redis option which means that we can not pass a redis connection. So if we use modelsCache and modelsMetadata, there will be two redis connections. So I modify this part and let redis option be supported, and through the lifetime of a request we can use one redis connection.

pantaovay added some commits Mar 30, 2016

Modify cache redis backend
# Changes

1. Modify cache redis backend, delete _PHCR
2. Cache redis backend supports "client" option, now you can pass a redis client

# Why

1. The redis cache backend use a special key _PHCR to store all keys used by cache. There is a issue: #10905 .According to @Green-Cat ,it is disabled in 2.0.x branch which is not true. If use _PHCR, every time we set a cache, we have to run sAdd and set commands. Store all keys in _PHCR will cause it expands quickly and this will influence performance.
2. The previous implementation mixes statsKey and prefix, it uses _PHCR as super prefix which is not needed.
3. The cache backend doesn't support redis option which means that we can not pass a redis connection. So if we use modelsCache and modelsMetadata, there will be two redis connections. So I modify this part and let redis option be supported, and through the lifetime of a request we can use one redis connection.
@pantaovay

This comment has been minimized.

Copy link
Author

pantaovay commented Mar 30, 2016

See #11452

@pantaovay

This comment has been minimized.

Copy link
Author

pantaovay commented Mar 30, 2016

@sergeyklay sergeyklay added this to the 2.1.0 milestone Mar 30, 2016

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Mar 30, 2016

I'll look into this as soon as I can

@@ -88,13 +99,21 @@ class Redis extends Backend implements BackendInterface
}

if !isset options["statsKey"] {
// Disable tracking of cached keys per default
let options["statsKey"] = "";
let options["statsKey"] = "_PHCR";

This comment has been minimized.

@sergeyklay

sergeyklay Apr 1, 2016

Member

Modify cache redis backend, delete _PHCR

As I remember it was done in pantaovay@6b88422

Why are we again added _PHCR ?

This comment has been minimized.

@pantaovay

pantaovay Apr 3, 2016

Author

Because it is inconsistent with the document. But current implementation is inconsistent with other cache backends😭. I will modify the document instead.

}

parent::__construct(frontend, options);
}

public function _getRedis()

This comment has been minimized.

@sergeyklay

sergeyklay Apr 1, 2016

Member

the _ prefix is inconsistent with common style of public Phalcon methods

This comment has been minimized.

@sergeyklay

sergeyklay Apr 1, 2016

Member

It seems to me getInternalHandler would be better

This comment has been minimized.

@pantaovay

pantaovay Apr 3, 2016

Author

Would private be better?


if keyName === null {
if !keyName {

This comment has been minimized.

@sergeyklay

sergeyklay Apr 1, 2016

Member

could you please explain this a bit more

This comment has been minimized.

@pantaovay

pantaovay Apr 3, 2016

Author

All the use of keyName in other methods is if !keyName, I think this is inconsistent. In libmemcached and memcache, if keyName === null is used in the save method too. Maybe all of them should be if !keyName?

This comment has been minimized.

@pantaovay

pantaovay Apr 3, 2016

Author

If user passs empty string or 0... An empty string is not allowed but 0 should be allowed.

I think the check should be

        if keyName === null || keyName === "" {
            let lastKey = this->_lastKey;
        } else {
            let lastKey = this->_prefix . keyName;
            let this->_lastKey = lastKey;
        }

This comment has been minimized.

@pantaovay

pantaovay Apr 3, 2016

Author

And I find that statsKey can not be set as "0"

}

/**
* Get the key from redis
*/
let keys = redis->sMembers(specialKey);
let keys = this->_getRedis()->sMembers(specialKey);

This comment has been minimized.

@sergeyklay

sergeyklay Apr 1, 2016

Member

anyway

this->_getRedis()->sMembers(specialKey);

is expensive than

var redis;
let redis = this->_getRedis();

redis->sMembers(specialKey);

This comment has been minimized.

@pantaovay

pantaovay Apr 3, 2016

Author

I don't know much about this. I will modify this part😊

@JCMais

This comment has been minimized.

Copy link

JCMais commented May 30, 2016

Is that PR still being worked on?

@sergeyklay sergeyklay removed this from the 2.1.0 milestone Jun 16, 2016

@tigran-a

This comment has been minimized.

Copy link

tigran-a commented Jul 1, 2016

Hi, just to let you know, this _PHCR is a pain for us, it eats all the memory from our Redis: the keys are stored with TTL, so after they expire, redis removes it, but _PHCR keeps growing regardless if the keys exist or not (there is no explicit key removal from php, we rely on TTL). Why not to modify all cache backends that they'd interpret null value as not to use the stats key?

@tigran-a

This comment has been minimized.

Copy link

tigran-a commented Jul 1, 2016

Sorry for using a language that is not related to the project, but this what we have to run from time to time to clean up that key without affecting the production, maybe it might be useful for someone else:

import redis
import socket
KEY = '_PHCR'
r = redis.Redis(host=socket.gethostname())
c = r.scard(KEY)
print("%s has %d elements"%(KEY, c)) 
print("Starting removing the key values. This can take some time") 
t = r.sscan(KEY)
if t[1]:
    r.srem(KEY, *t[1])
while t[0]:
    t = r.sscan(KEY, t[0])
    if t[1]:
        r.srem(KEY, *t[1])
c = r.scard(KEY)
print("%s has %d elements"%(KEY, c)) 
@wcoppens

This comment has been minimized.

Copy link

wcoppens commented Jul 11, 2016

Just to let you know, the _PHCR prefix really is annoying. We for instance use the redis cache across multiple applications. Phalcon in our case created the redis cache data since it acts as a receiver of measurement data. In Phalcon 1.x with the incubator package we did not have the prefix behavior and so all the cached data in Redis is not prefixed. However since we can't stay legacy we are forced to upgrade resulting in upgrading our whole application stack to support the prefix... It would really help us if there would come a solution in which the key is optional.

@sjinks sjinks closed this Apr 26, 2017

@sjinks sjinks reopened this Apr 26, 2017

@aat2703

This comment has been minimized.

Copy link

aat2703 commented Mar 7, 2018

Will this be added to 3.3.3? Really Looking forward to it 😁

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Oct 31, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 1, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 1, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 1, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 1, 2018

@niden niden referenced this pull request Nov 1, 2018

Merged

PR-11608 Modify redis backend #13563

3 of 3 tasks complete
@niden

This comment has been minimized.

Copy link
Member

niden commented Nov 1, 2018

Closing in favor of #13563

@niden niden closed this Nov 1, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 3, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 3, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 3, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 3, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 3, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 3, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 3, 2018

niden added a commit to niden/cphalcon that referenced this pull request Nov 4, 2018

[phalcon#11608] - Merge branch '4.0.x' into feature/T11608-modify-red…
…is-backend

* 4.0.x:
  [PR12189] - Removed unused variables; Corrected missing use statement
  [PR12189] - Corrected typo
  [PR12189] - Minor changes based on review
  [PR12189] - Modified header class and corrected tests
  [PR12189] - Corrected logic and test
  [PR12189] - Fixed tests
  [PR12189] - Added test for hasHeader
  [PR12189] - Updated changelog
  [PR12189] - Added hasHeader method
  Minor CI's config refactor and improvements

niden added a commit that referenced this pull request Nov 4, 2018

Merge branch 'niden-feature/T11608-modify-redis-backend' into 4.0.x
* niden-feature/T11608-modify-redis-backend: (28 commits)
  [#11608] - Removed unused variables
  [#11608] - Reverting instantiation of the client
  [#11608] - Removed '_' from variables and methods
  [#11608] - Refactoring on the connect to check if the object is already initialized/connected
  [#11608] - Refactoring of the getClient()
  [#11608] - Fixed tabs
  [#11608] - Corrections based on review
  [#11608] - Corrected variable name
  [#11608] - Corrected costructor and changelog
  [#11608] - Updated the changelog
  [#11608] - Corrected tests
  [#11608] - Fixed typos
  [#11608] - More corrections
  [#11608] - Modifications to the class; Adjusted tests
  [#11608] - Changed the comparison for the key on save
  [#11608] - More corrections
  [#11608] - Changed the instatiation of redis
  [#11608] - Trying protected getClient
  [#11608] - Changed the getClient slightly
  [#11608] - Corrections to variables
  ...

@niden niden referenced this pull request Nov 4, 2018

Open

Update 4.x Documents #1935

0 of 10 tasks complete

niden added a commit to niden/cphalcon that referenced this pull request Nov 11, 2018

Merge branch '4.0.x' into T13543-add-more-pdo-types-3
* 4.0.x: (33 commits)
  Updated stale to not close issues labelled as bugs
  Use latest Zephir Parser
  [phalcon#11608] - Removed unused variables
  [phalcon#11608] - Reverting instantiation of the client
  [phalcon#11608] - Removed '_' from variables and methods
  [phalcon#11608] - Refactoring on the connect to check if the object is already initialized/connected
  [phalcon#11608] - Refactoring of the getClient()
  [phalcon#11608] - Fixed tabs
  [phalcon#11608] - Corrections based on review
  [phalcon#10532] - Corrected variable
  [phalcon#10532] - Updated changelog
  [phalcon#10532] - Added case insensitive column map in model
  [phalcon#11608] - Corrected variable name
  [phalcon#11608] - Corrected costructor and changelog
  [phalcon#11608] - Updated the changelog
  [phalcon#11608] - Corrected tests
  [phalcon#11608] - Fixed typos
  [phalcon#11608] - More corrections
  [phalcon#11608] - Modifications to the class; Adjusted tests
  [phalcon#11608] - Changed the comparison for the key on save
  ...

niden added a commit to niden/cphalcon that referenced this pull request Nov 11, 2018

Merge branch '4.0.x' into T13578-setup-phalcon-environment
* 4.0.x: (33 commits)
  Updated stale to not close issues labelled as bugs
  Use latest Zephir Parser
  [phalcon#11608] - Removed unused variables
  [phalcon#11608] - Reverting instantiation of the client
  [phalcon#11608] - Removed '_' from variables and methods
  [phalcon#11608] - Refactoring on the connect to check if the object is already initialized/connected
  [phalcon#11608] - Refactoring of the getClient()
  [phalcon#11608] - Fixed tabs
  [phalcon#11608] - Corrections based on review
  [phalcon#10532] - Corrected variable
  [phalcon#10532] - Updated changelog
  [phalcon#10532] - Added case insensitive column map in model
  [phalcon#11608] - Corrected variable name
  [phalcon#11608] - Corrected costructor and changelog
  [phalcon#11608] - Updated the changelog
  [phalcon#11608] - Corrected tests
  [phalcon#11608] - Fixed typos
  [phalcon#11608] - More corrections
  [phalcon#11608] - Modifications to the class; Adjusted tests
  [phalcon#11608] - Changed the comparison for the key on save
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment