Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Cache] Fix Predis client cluster with pipeline#23237
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 with minor comments
| $ids =array(); | ||
| if ($this->redisinstanceof \Predis\Client) { | ||
| if ($this->redisinstanceof \Predis\Client && !($this->redis->getConnection()instanceof ClusterInterface)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No need for the parentheses.
| }elseif ($this->redisinstanceof \RedisCluster) { | ||
| // phpredis doesn't support pipelining with RedisCluster | ||
| }elseif ($this->redisinstanceof \RedisCluster || ( | ||
| $this->redisinstanceof \Predis\Client &&$this->redis->getConnection()instanceof ClusterInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be on one line.
fabpot commentedJun 20, 2017
@flolivaud Can you change the base to the lowest possible version? |
flolivaud commentedJun 20, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@fabpot Done |
keradus commentedJun 21, 2017
when lowering the branch on gh interface, one needs to rebase the branch as well, as if not, he ends up with all commits from original base branch in his PR |
flolivaud commentedJun 21, 2017
keradus commentedJun 21, 2017
all bugfixes deserve a test. please add one |
nicolas-grekas commentedJun 21, 2017
Note that in this case, having a test might be complicated... |
flolivaud commentedJun 21, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes it is. I try to fix the test to connect in cluster mode but i don't know how to do more. |
69c12cc to81a9efcCompare| }elseif ($this->redisinstanceof \RedisCluster) { | ||
| // phpredis doesn't support pipelining with RedisCluster | ||
| }elseif ($this->redisinstanceof \RedisCluster || ($this->redisinstanceof \Predis\Client &&$this->redis->getConnection()instanceof ClusterInterface)) { | ||
| // phpredis& predisdoesn't support pipelining with RedisCluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
s/doesn't/don't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry. Done. :)
fabpot commentedJun 21, 2017
Thank you@flolivaud. |
This PR was merged into the 3.3 branch.Discussion----------[Cache] Fix Predis client cluster with pipeline| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aCommits-------a85d5b0 Fix Predis client cluster with pipeline
flolivaud commentedJun 22, 2017
@fabpot the fix is actually on the 3.3 branch, but need to be on 3.3+ branches (master included). |
xabbuh commentedJun 22, 2017
@flolivaud branches are merged up regularly so each fix will eventually land in all still maintained branches |
Uh oh!
There was an error while loading.Please reload this page.