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

Upgrade tests and laravel 6 #79

Merged
merged 9 commits into from
Dec 13, 2019
Merged

Upgrade tests and laravel 6 #79

merged 9 commits into from
Dec 13, 2019

Conversation

rbruhn
Copy link

@rbruhn rbruhn commented Dec 10, 2019

This updates LadaCache to Laravel 6 using the work done by @tof06 but with tests working. Changes are as follows:

  1. RedisTest: was failing due to deprecation.
  2. Cache.php: flush method for collecting keys was changed to scan as it's less resource intensive on production sites.
Warning: consider KEYS as a command that should only be used in production environments with extreme care. It may ruin performance when it is executed against large databases. 
  1. Cache.php: Redis automatically adds the options->prefix when preforming a delete. When retrieving the keys, the full key with Redis prefix is returned (i.e. "laravel_database_lada:tag"). This prefix needs to be stripped for deletion to occur.

I added a variable for retrieving the prefix using config() for the scan since Redis doesn't automatically attach the prefix when scanning. There is an issue about this that is years old.

@rbruhn rbruhn mentioned this pull request Dec 10, 2019
@@ -120,10 +120,11 @@ public function get($key)
*/
public function flush()
{
$keys = $this->redis->keys($this->redis->prefix('*'));
$prefix = config('database.redis.options.prefix');
$keys = $this->redis->scan(null, $prefix . $this->redis->prefix('*'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a problem with scan method. It only return partial results (default 10). We should loop on cursor here if we want to flush all cache.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I wonder if this commit is still useful ?
Because it just removes the redis prefix in testing environment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking this. I was only doing a couple keys and didn't know there is a limit to scan. I can change this code later today.
Cache.php is the source code. This is the flush function that's called by tests and source. I tested using lada-cache:flush from artisan and it worked. Did it not work for you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

artisan lada-cache:flush only removes the first 10 keys... If you have more keys, you need to run it several times.
I modified testFlush like this :

public function testFlush()
    {
        for ($i=0; $i < 30; $i++) {
            $this->cache->set('key'.$i, ['tag'], 'data'.$i);
        }
        $this->cache->flush();

        for ($i=0; $i<30; $i++) {
            $this->assertFalse($this->cache->has('key'.$i));
        }
    }

and it didn't pass.

I tried to change the Cache::flush method, but wasn't be able to get next cursor position after scan (and the scan signature method is also depending on redis client.. predis and phpredis didn't have the same signature... anyway, predis is deprecated now...)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will start working on this now. What do you mean by the signature method being different on predis vs phpredis? I see an old issue on this regarding Laravel but it apparently was never merged. Can you show me the method?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For methods that are not overloaded in Illuminate\Redis\Connection class or its children, the command is sent directly to underlying client (phpredis or predis)
1st problem :
phpredis scan method takes an iterator (or cursor or whatever you called it), an optional pattern and an optional count.
predis seems to send the raw SCAN command directly to REDIS server (from my test, I didn't dig deeper in source), and therefore, need a MATCH before pattern (redis-cli scan command is SCAN <cursor> [MATCH <pattern>] [COUNT])

2nd problem:
In predis, 1st parameter of scan method should be passed by reference, to allow the method to update its value with the new cursor position. But because this method is called by a magic __call in Laravel, I'm not sure that the reference is kept (at least, this parameter is not updated after a 1st call).

Anyway, both problem are Laravel problem on the Redis Connection abstraction, not really a ladacache problem.

I know KEYS command is time consuming, but it's far simpler than using SCAN, IMHO. And users should not flush cache often. The invalidation mechanism used in ladacache works well

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @tof06 - the flush command is only for development purposes and should not be used as part of the application logic. Therefore, speed is second priority here.

@rbruhn
Copy link
Author

rbruhn commented Dec 12, 2019

I see what you mean @tof06. I changed it back to using keys method. Can you check it out and see if it works on your system now?

@spiritix spiritix changed the base branch from master to 74-laravel6-support December 13, 2019 10:15
@spiritix spiritix changed the base branch from 74-laravel6-support to tmp-l6-support December 13, 2019 10:17
@spiritix spiritix merged commit 544dbd6 into spiritix:tmp-l6-support Dec 13, 2019
@spiritix
Copy link
Owner

I am continuing development for Laravel 6 on this branch: https://github.com/spiritix/lada-cache/tree/74-laravel6-support

Please feel free to provide feedback and fixes.

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

3 participants