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

Fix Correspond to RedisCluster from RedisEngine::clear() #10781

Merged
merged 2 commits into from Jun 22, 2017

Conversation

Projects
None yet
3 participants
@h-moriya
Contributor

h-moriya commented Jun 15, 2017

Redis cluster will fail with Multiple Keys

redis-cli :

> del kye1 kye2
> (error) CROSSSLOT Keys in request don't hash to the same slot

Execution of the RedisEngine::clear() makes it true, but the key is not deleted

@markstory markstory added this to the 3.4.8 milestone Jun 15, 2017

@markstory markstory added the cache label Jun 15, 2017

@markstory

The test suite is failing. Could you fix the failing test?

@markstory markstory assigned markstory and unassigned markstory Jun 15, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Jun 16, 2017

Codecov Report

Merging #10781 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #10781      +/-   ##
============================================
+ Coverage      94.9%   94.91%   +0.01%     
- Complexity    12121    12122       +1     
============================================
  Files           422      422              
  Lines         30109    30114       +5     
============================================
+ Hits          28575    28583       +8     
+ Misses         1534     1531       -3
Impacted Files Coverage Δ Complexity Δ
src/Cache/Engine/RedisEngine.php 93.5% <100%> (+0.17%) 33 <0> (+1) ⬆️
src/Http/ServerRequestFactory.php 100% <0%> (ø) 33% <0%> (ø) ⬇️
src/Cache/Engine/FileEngine.php 86.59% <0%> (+1.11%) 72% <0%> (ø) ⬇️
src/Cache/CacheRegistry.php 100% <0%> (+4.16%) 11% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48e0393...fd7a29d. Read the comment docs.

@h-moriya

This comment has been minimized.

Contributor

h-moriya commented Jun 16, 2017

Sorry, Fixed the mistake

return !in_array(false, $this->deleteMany($keys));
$result = [];
foreach ($keys as $key) {
$result[] = $this->_Redis->delete($key) > 0;

This comment has been minimized.

@markstory

markstory Jun 16, 2017

Member

It looks like delete() takes an array of keys. Why are you calling delete N times?

This comment has been minimized.

@h-moriya

h-moriya Jun 19, 2017

Contributor

because delete() is to create the Multiple Keys command

e.g.


php

$keys = ['KEY1', 'KEY2', 'KEY3'];
$this->_Redis->delete($keys)

redis-cli

del KEY1 KEY2 KEY3

php

$keys = ['KEY1', 'KEY2', 'KEY3'];
foreach ($keys as $key) {
    $this->_Redis->delete($key);
}

redis-cli

del KEY1
del KEY2
del KEY3

This comment has been minimized.

@markstory

markstory Jun 19, 2017

Member

Ok. Which is the source of the CROSSLOT issue you're facing.

@markstory markstory self-assigned this Jun 19, 2017

@markstory markstory modified the milestones: 3.4.8, 3.4.9 Jun 20, 2017

@markstory markstory merged commit 42d4388 into cakephp:master Jun 22, 2017

5 checks passed

codecov/patch 100% of diff hit (target 94.9%)
Details
codecov/project 94.91% (+0.01%) compared to 48e0393
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.

@h-moriya h-moriya deleted the h-moriya:fix_Redis branch Jun 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment