nrk / redis-lua

A Lua client library for the redis key value storage system.
MIT License
731 stars 239 forks source link

Docs clarification request #6

Open agladysh opened 13 years ago

agladysh commented 13 years ago

Please include somewhere in documentation answers on these "Frequently asked" questions (change answers if I did not guess right):

Q: Does redis-lua attempt to reestablish connection if it is lost? A: No

Q: Is transaction() inside pipeline() supported? A: No

Q: Is it possible to use return nil, error_message strategy instead of throwing error() on every error? A: No.

agladysh commented 13 years ago

I want to clarify my request, as there are some comments that this ticket is rude. I apologize if that is the case.

The text above is not intended as critique of redis-lua or a help request.

I had these questions in mind when I came to evaluate the library, and I had to dig through the source to find answers. Thus — I think that it is a good idea to put them into docs.

But besides that I do not view these issues to be bugs. (Despite the fact that first and third issue together prevent me from using redis-lua.) These issues are not bugs, but are valid design decisions.

agladysh commented 13 years ago

Now, to be more constructive, what improvements can I suggest, so redis-lua is useful for me in may particular situation?

Two things come to my mind:

  1. Implement auto-reconnect feature. (Not necessary a good idea — it is may be outside of the scope of the library.)

-- or --

  1. Let user to specify optional error callback, and pass to that callback some clear indication that the error is that the connection is lost (for example, a documented error message string). This would lead to some runtime overhead at caller though...

From cursory glance at the redis-lua source I do not think that it is feasible to rewrite it to return nil, err idiom. (As much as I would want this.)

agladysh commented 13 years ago

Note that I still do not explicitly ask for help (though I would appreciate it), and this ticket should be considered as a random voice from the crowd. The problem with above changes is that they will probably affect the library design, and I can not say if it would be a good idea.

nrk commented 13 years ago

Sorry for the late reply but between work and conferences I couldn't get much done with other stuff. Now onto your questions:

  1. Plain automatic reconnection wouldn't work with servers requiring a password (and thus the need to issue an AUTH command), but this problem could be solved with a callback that gets called soon after the connection is estabilished. The same cannot be said for SELECT since the client doesn't keep track of the currently selected database.
  2. No, but it would be indeed a nice-to-have feature. Added to my TODO list for now.
  3. I myself am not happy with using error() to notify the user of server-side errors. I actually thought about using return nil, err while rewriting redis-lua, but then I decided to wait since I couldn't find a way to get it to work when using pipelines or transactions. Let's suppose response.error used return nil, err instead of raising an error, how would you store the errors inside the table that gets populated with the results of a pipeline? Right now it would end up like this (see also my experiment_silent_errors branch):

    local replies = redis:pipeline(function(p)
       p:set('foo', 'bar')
       p:lpush('foo', 0)  -- wrong, Redis returns an -ERR
       p:get('foo')
    end)
    -- { 1=true, 3=bar }

    Do you have an idea that comes to mind? By the way, for almost all the other kind of errors (the client-side ones at least) I think that using error() is still the right way to handle things.

As for the lack of documentation, it's indeed not a good thing that definitely needs to be fixed :-(

agladysh commented 13 years ago

1) Other way to solve auto-reconnect problem is to lay it on user's shoulders. Just fail with clear indication of what is happened, so user can reconnect himself when needed.

2) About nil, err in tables: see recent discussion about hiredis bindings in Lua ML. See my lua-hiredis for one of possible error handling strategies in this case. Silent errors are not acceptable, of course.

3) From my point of view, error() in library code is not a good thing. But if you will give me a way to override error() that redis-lua calls, I'll probably be fine. (That is call connection.error(), not a global, and let user to override it.)

nrk commented 13 years ago

1) Following the third point, a solution might consist in having a user overridable callback with the following arguments: _errortype (value), _errormessage (string) and _connectionclosed (bool). If _connectionclosed is true, then the user should handle everything needed to reconnect.

2) I missed the lua-hiredis announcement, I'll take a peek at it later. As for silent, I've probably used the wrong term: it's silent as in it doesn't raise any runtime error, but the intention is obviously that the error should be somehow returned to the user.

3) Sure, that should be easy enough to obtain without twisting the current library code and breaking the public API.

agladysh commented 13 years ago

1) In this case you should (a) be able to guarantee that it is safe to reconnect when from inside error handler — and give user means to do that; and (b) store last used DB id, or user will be forced to wrap your connection to capture it.