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

Feature request - read timeout is useless #94

Open
vsespb opened this issue Aug 10, 2014 · 5 comments
Open

Feature request - read timeout is useless #94

vsespb opened this issue Aug 10, 2014 · 5 comments

Comments

@vsespb
Copy link
Contributor

vsespb commented Aug 10, 2014

read_timeout should be used to detect that something nastly is happened with Redis and display proper error message as soon as posisble.
for example we might use
GET with timeout 2 sec
and SET with timeout 2 sec

in case 2 sec is maximum deadline for our server/load.

However blocking commands are different. They have server timeout as well.

So BLPOP with server timeout 10 sec and client read_timeout 2 secs won't work.

Thus we need use timeout $CLIENT_TIMEOUT=2 sec for GET/SET/etc, unblocking.
and timeout $CLIENT_TIMEOUT + $BLPOP_TIMEOUT with BLPOP somekey, $BLPOP_TIMEOUT.

we have now to use different connections.

ie. would be good to have different timeout settings for blocking and non-blocking commands or to auto-adjust timeouts for blocking commands with server timeout.

@ikruglov
Copy link

Hi, I would argue that we should be clear in the documentation about this case. Implementation of suggested logic would require some magic of $CLIENT_TIMEOUT + $BLPOP_TIMEOUT which usually not a great idea and error prone.

@ikruglov
Copy link

issue #89 is related to this one

@vsespb
Copy link
Contributor Author

vsespb commented Aug 10, 2014

Implementation of suggested logic would require some magic

maybe, yes, I agree.

but anyway, I think there should be way to specify different timeouts.

like maybe mutators

$redis->get("key)
$redis->timeout($timeout_for_blpop)->blpop("key")

or dynamic variables

$redis->get("key")

override_redis_global_timeouts $mytimeout => sub { # sets local $_global_redis_timeout to $mytimeout
   $redis->blpop;
};

or maybe just Redis->new(read_timeout => 123, timeout_for_blocking_commands => 456)

@vsespb
Copy link
Contributor Author

vsespb commented Aug 10, 2014

or maybe a syntax sugar like this.

$redis->blpop("list1", "list2", $server_timeout);

vs

$redis->blpop("list1", "list2", use_client_timeout($mytimeout), $server_timeout);

use_client_timeout will return a blessed object of a special type and sub blpop() will detect it in arguments, use it, and remove from original arguments.

@dams
Copy link
Member

dams commented Oct 15, 2014

I agree, we should make it possible to set the 2 kinds of timeouts. From what you suggested, I prefer this one:

Redis->new(read_timeout => 123, timeout_for_blocking_commands => 456)

It's simple to understand, and general enough to require less coding, I think.

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

3 participants