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

Using setOption on redis array causes immediate connection #504

Closed
denveloper opened this issue Sep 5, 2014 · 11 comments
Closed

Using setOption on redis array causes immediate connection #504

denveloper opened this issue Sep 5, 2014 · 11 comments
Milestone

Comments

@denveloper
Copy link

     $options = [
        'retry_interval' => 10,
        'connect_timeout' => 2,
        'lazy_connect' => true,
    ];       

$servers = ["validserver:6379", "invaliddnsname:6379"];
$this->redisArray = new RedisArray($servers, $options);

$this->redisArray->setOption(Redis::OPT_SERIALIZER, Redis::SERIALIZER_PHP); <- redisarray will try to connect here

why this is bad. I setup everything as lazy in class constructor and expect to catch exceptions only when required server will be called. E.g. if I comment out this line and do call set that will land on "validserver" nothing bad will happen.

Is there reason for such behavior?

@gonzaloserrano
Copy link

Hi there,

I don't know if it's related but i just created a multiple shard Redis Array cluster setting array('lazy_connect' => true) as 2nd parameter in the constructor, and just after instancing the RedisArray class i'm getting TCP packets (using tcpdump) in all the servers, so i don't think the lazy stuff is working properly.

Any hints about this?

@michael-grunder
Copy link
Member

Thanks for the reports guys, I'll look into this for you. I'll commit any fix into develop and then it'll come out in a future release for things like PECL.

Cheers!
Mike

@ghost
Copy link

ghost commented Feb 5, 2015

Hi, could you investigate what causes RedisArray to connect to all servers even with lazyness activated? Thanks!

@michael-grunder
Copy link
Member

The reason it's connecting is that methods like getOption are passed through to the individual Redis objects that handle connections to specific nodes. In order to avoid this, I'll have to instrument the Redis object proper in such a way that it will inherit the lazy connection mechanism.

michael-grunder added a commit that referenced this issue Feb 7, 2015
Some methods don't need to automatically connect to Redis if
called either with a standard Redis object or from the context
of a RedisArray object.

This includes stuff like getOption(), setOption(), etc which don't
actually need to connect as they don't need anything over the wire.

Addresses #504
@michael-grunder
Copy link
Member

Can you please try the branch feature/inherit_lazy_commit. Methods that don't need to talk over the wire shouldn't force a connection now.

Cheers!
Mike

@gonzaloserrano
Copy link

I'll try tomorrow and will report results, thanks!

@gonzaloserrano
Copy link

Hi,

I cloned the project, did checkout of the feature/inherit_lazy_commitbranch, cofigured, compiled, installed and executed something like this:

<?php

// need to boot these 5 docker containers first

$serverList = array(
    'redis1',
    'redis2',
    'redis3',
    'redis4',
    'redis5',
);

testLazyConnection($serverList);

function testLazyConnection($serverList)
{
    echo "\n\ntest lazy connection with tcpdump in redis containers\n";

    // - lazy connecting is not working, see https://github.com/phpredis/phpredis/issues/504
    // - in redis container # tcpdump tcp -nS port 6379 -i eth0
    $arguments = array('lazy_connect' => true);
    new RedisArray($serverList, $arguments);
}

Doing a tcpdump -n dst port 6379 in all the involved redis servers gave me an output like:

16:26:12.134194 IP 172.17.0.25.52811 > 172.17.0.22.6379: Flags [S], seq 2520188836, win 29200, options [mss 1460,sackOK,TS val 8308163 ecr 0,nop,wscale 7], length 0
16:26:12.136822 IP 172.17.0.25.52811 > 172.17.0.22.6379: Flags [.], ack 3464379299, win 229, options [nop,nop,TS val 8308163 ecr 8308163], length 0
16:26:12.139257 IP 172.17.0.25.52811 > 172.17.0.22.6379: Flags [P.], seq 0:6, ack 1, win 229, options [nop,nop,TS val 8308163 ecr 8308163], length 6
16:26:12.139437 IP 172.17.0.25.52811 > 172.17.0.22.6379: Flags [F.], seq 6, ack 1, win 229, options [nop,nop,TS val 8308163 ecr 8308163], length 0
16:26:12.140361 IP 172.17.0.25.52811 > 172.17.0.22.6379: Flags [R], seq 2520188843, win 0, length 0
16:26:12.150482 IP 172.17.0.25.52811 > 172.17.0.22.6379: Flags [R], seq 2520188844, win 0, length 0

As far as i understand there shouldn't be any output in this tcpdump executions since the test does not execute any redis operation yet, am i right?

Thanks!

@michael-grunder
Copy link
Member

Hey,

I'll try to replicate your issue using the tools you are. Honestly the way I did it was to spin up a redis array and simply going to the redis server in question and doing a CLIENT LIST. When I did this, I only saw the client for redis-cli.

You're testing at a lower level though so I'll look into it.

Cheers,
Mike

@gonzaloserrano
Copy link

That's very interesting indeed. I didn't check CLIENT LIST, just that TCP packets where arriving from the client to the servers, but i don't have the knowledge (yet :P) to interpret them.

@bryanlatten
Copy link

bryanlatten commented Nov 23, 2016

re-triggering this thread, any indication when the lazy branch will make it into the stable extension? It's a big deal!

@yatsukhnenko yatsukhnenko added this to the 3.1.0 milestone Nov 23, 2016
@yatsukhnenko
Copy link
Member

@bryanlatten, could you test fix from PR #1032?

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

No branches or pull requests

5 participants