tests: add reconnect test #68
Conversation
| } else if (acc->cc->err && strcmp(acc->cc->errstr, REDIS_ENOCLUSTER) == 0) { | ||
| //printf("[no cluster]\n"); | ||
| acc->cc->err = 0; |
There was a problem hiding this comment.
I think printing [no cluster] here is a good idea. Then we can check for it in the output too.
For minimum surprise, we could accept Redis standalone in clusterclient_async.c too and add the the same printout there. Makes sense?
There was a problem hiding this comment.
I've added the [no cluster] message now.
Regarding clusterclient_async.c: This test is using redisClusterAsyncCommand, and this function needs the slot table setup properly. So it won't work in standalone mode.
Before discovering the redisClusterAsyncCommandToNode function I was already "faking" a slots table in the standalone mode to be able to use all functions, but I'm not sure if we want this feature in the core hiredis-cluster library. If there are errors, it will try to refresh the slots table again, etc. - not sure how to implement that in a clean way. But support for both cluster and standalone mode is definitely important, currently I'm handling it by checking (and ignoring) the error when connecting and using the *ToNode functions exclusively when in standalone mode.
There was a problem hiding this comment.
OK. Sounds good. I don't really know what works in standalone mode and what doesn't, so the first step may be to figure it out and document it.
Then, I think it will be safe to set up fake slot table (with a single node for all slots) every time CLUSTER SLOTS returns the "no cluster" error. What can go wrong with this approach?
Users may want to use standalone mode for development and cluster for production, so it's good if everything works without changes to application code.
There was a problem hiding this comment.
What can go wrong with this approach?
For example in the redisClusterAsyncRetryCallback we're calling cluster_update_route() if the reply is NULL, which will fail in the standalone mode. Maybe we should store a boolean whether we're in standalone or cluster mode, and turn cluster_update_route() in a no-op in standalone mode. OTOH, technically it won't fail if we intercept the "no cluster" error, but we also don't need to submit the CLUSTER SLOTS request a second time.
Currently we're doing something similar to above in our client application (https://github.com/performancecopilot/pcp/blob/5eca7119a83bf9249057dd55ce32918b4c658ceb/src/libpcp_web/src/slots.c#L311-L346 and https://github.com/performancecopilot/pcp/blob/5eca7119a83bf9249057dd55ce32918b4c658ceb/src/libpcp_web/src/slots.c#L529-L530), but it'd be nice to have it integrated in hiredis-cluster directly. I'll think about something, but probably not before next year.
There was a problem hiding this comment.
I created #69. Feel free to update (or comment if you can't update...).
There was a problem hiding this comment.
Hey, is there any trap to use redisClusterCommandToNode for standlone mode ? I really need to support both standalone and cluster mode right now
as promised last week, this new test examines
redisClusterAsyncContextfor cluster andredisAsyncContextfor standalone) :)The
clusterclient_reconnect_async.cis quite similar to theclusterclient_async.c, except it sends the next command after receiving a reply from the previous command. It also works for standalone Redis nodes (without cluster mode), and uses the redisClusterAsyncCommandToNode function to send the command to the first node. If it receives any I/O error, the program performs a reconnect.Naming is hard though and I'm open to any renames, or other suggestions/improvements of course.