Discussion: Get rid of ZkClient class about redis_failover

[ryanlecompte]

I'd love to get rid of that wrapper class, although I'm not sure if it's possible to do so. I believe the original reason I introduced it was to properly rebuild the underlying ZK instance on any connectivity error. @slyphon, do you see any merit in wrapping the ZK instance like this? It may just be overly defensive.

[eric]

For context, here is a link to lib/redis_failover/zk_client.rb.

[slyphon]

(Ok, this is a little scattershot, i've had a long day. Trying to get a passport for a 3 year old is a serious ordeal :))

It's hard to say. Once you lose your connection (i.e. SESSION_EXPIRED), even if you use reopen on the client, you still have to set your watches again (i.e. zk.stat('/path', :watch => true)), and if you recreate the client, you have to re-register your watchers, so there is a fair amount of work to be done in these cases. There's an argument to be made that you should synchronize when doing this to ensure that the user doesn't see an effect to changing the underlying object.

One thing you should definately do on line 87 is close the old client. You've set up callback hooks, and if that connection re-establishes itself, it will deliver events to the blocks you've registered in on_session_expiration and on_session_recovered.

In the 0.9 release of ZK (which should be forthcoming early this week), I'll mix a module into the the exceptions you have listed here so that you can just catch ZK::Exceptions::InterruptedSession and then check what kind_of? exception it is (if you want to) the specific class of error raised (just for convenience).

After looking at the code, I made a fork and a branch, and will make a pull request. I do think on reviewing that the perform_with_reconnect is probably too defensive, and I'd be concerned about ZK::Client::Base#with_lock interaction with that code. You're already catching exceptions here and here. It seems like the only place you're exposed to a ZK related error is in Client#fetch_nodes, so it might make sense to rescue/retry the relevant ZK errors there.

[eric]

So, it seems to me that if you called reopen in an on_expired_session callback, and called all your :watch => true commands in an on_connected callback, you would get the reconnection logic with less work, right?

One of the reasons why I'm interested in removing this, is as it is now, we're requiring a new ZkClient instance for each redis client created, which needs to be one per thread (if you are accessing redis on multiple threads).

It would be nice to be able to remove the need for the extra retry logic so it could work on a normal ZK client and just have each client register for their own on_connected callback (the ZK gem allows more than one to be registered), and have everything else Just Work.

[ryanlecompte]

@eric / @slyphon : I just pushed a side branch that removes the zk_client.rb wrapper and just does what Eric mentioned in in his last comment. Unfortunately, it just doesn't work as we would expect. When the session expires, I call #reopen. Next, when I get the :connected callback, I issue a #stat with :watch => true. After that, I no longer get watcher events fired from the Node Manager. The #reopen just doesn't seem to work in this case.

[ryanlecompte]

Here's the branch for this: https://github.com/ryanlecompte/redis_failover/compare/no_zk_wrapper

[ryanlecompte]

I just pushed to master a change that removes the zk_client.rb entirely. After consulting with @slyphon, I've upgraded to version 0.9 of ZK and also cleaned up handling of the ZK events fired from the ZK event handler thread. I now handle them in a separate Queue in RedisFailover::Client so that we don't block the main ZK event handler thread. I also cleaned up NodeManager to use ZK directly and properly rescue the new ZK::Exceptions::InterruptedSession.

[eric]

Sweet!