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

wait_until_loaded option #98

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

wait_until_loaded option #98

wants to merge 1 commit into from

Conversation

melo
Copy link
Member

@melo melo commented Oct 4, 2014

An attempt at solving #93.

Not a perfect solution by large, and on top of that I don't have a large enough dataset on my laptop to test this.

Also, not sure about the placement of the "fix", the auth() command is used before-hand, not sure if that command works while LOADING.

Anyway, if someone has large datasets, see if this works for you.

@vsespb
Copy link
Contributor

vsespb commented Oct 12, 2014

Anyway, if someone has large datasets, see if this works for you.

I'll test a bit later

Also, not sure about the placement of the "fix", the auth() command is used before-hand, not sure if that command works while LOADING.

not sure either

if someone has large datasets

btw there is a great test in R:F

https://github.com/shogo82148/Redis-Fast/pull/23/files#diff-1

@vsespb
Copy link
Contributor

vsespb commented Oct 12, 2014

also I am thinking maybe it should stop trying after some timeout.. (maybe use reconnect option as timeout?)

@vsespb
Copy link
Contributor

vsespb commented Oct 13, 2014

  1. In order to test fix I had to fix another problem. I was getting "Connection reset by peer" from here:
diff --git a/lib/Redis.pm b/lib/Redis.pm
index eade528..42840af 100644
--- a/lib/Redis.pm
+++ b/lib/Redis.pm
@@ -772,7 +772,7 @@ sub __read_line {
   my $sock = $self->{sock};

   my $data = $self->__read_line_raw;
-  croak("Error while reading from Redis server: $!")
+  $self->__throw_reconnect("Error while reading from Redis server: $!")
     unless defined $data;

   chomp $data;

when I restarted redis server (most of time, LOADING error was less frequent).

Then I tested fix - yes it works.

  1. About AUTH command. Seems it should be OK:

https://github.com/antirez/redis/blob/38a5db6c9ac21812e4942d7016e4e58429f9ab09/src/redis.c#L2236
https://github.com/antirez/redis/blob/38a5db6c9ac21812e4942d7016e4e58429f9ab09/src/redis.c#L110
https://github.com/antirez/redis/blob/38a5db6c9ac21812e4942d7016e4e58429f9ab09/src/redis.c#L225

  1. I propose to add additional wait_until_loaded_timeout option to control max possible wait period.
    I can send PR, if it's ok

So:
wait_until_loaded => TRUE|FALSE
if wait_until_loaded is TRUE, then wait_until_loaded_timeout is timeout in seconds. Defaults to 120.

OK?

note that I have filing that names wait_until_loaded and wait_until_loaded_timeout are not consistent with other option names. But I cannot suggest anything. It's up to you.

  1. Sad that the test is missing.

@dams
Copy link
Member

dams commented Jan 27, 2015

coming back to this issue, it seems that

-  croak("Error while reading from Redis server: $!")
+  $self->__throw_reconnect("Error while reading from Redis server: $!")

would invalidate the documentation about reconnect:

Be aware that read errors will always thrown an exception, and will not trigger
a retry until the new command is sent.

It seems that in most of the case (so not in wait_until_loaded), we want it to croak and not retry. So the code should realize that it's in specific case of wait_until_loaded

@dams dams force-pushed the master branch 2 times, most recently from e5ccb43 to 0dfcf02 Compare August 17, 2020 08:16
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.

3 participants