Fix: Don't assume the zk client hasn't yet connected #15
Conversation
Was triggering a compiler warning
@test methods apparently need to be nested inside at least one inner class or else they'll be ignored... *shrug*
If the user supplies their own zk client to cluster.join (or cluster.connect), and their zk client has already established a session, then the cluster will register a watch for a SyncConnected event but never receive one, since it has already fired long ago, before the cluster started listening.
|
Actually, this fix is unsafe because the access to onConnect isn't synchronized across the user thread and the zk event thread. I'll revisit this in the morning... |
|
Yeah ... right. ¯(°_o)/¯ |
|
Ok, I just went ahead and synchronized all of connectionWatcher.process so that the zk event thread and user thread won't collide. Let me know what you think—I still can't decide whether reusing connectionWatcher.process in the first place is a dirty hack or a simple and elegant solution... |
|
Yes, the use of connectionWatcher.process smells like a dirty hack. But if the simplest solution that works, that's fine.
Finally, you should step back to evaluate if the cost of synchronizing all access to connectionWatcher() is worth just to have the client issue its own zookeeper client. Sorry, I still don't see a good reason for this besides testing or saving a connection. The lock synchronization will reduce the throughput so you should have a very good reason to this. Maybe I am just plain dumb, but the only reason I came up with for this change was enable testing (passing a mock object) and re-use the same zookeeper connection to store/load ordasity data and other unrelated app data. |
|
This fix is unsafe with |
If the user supplies their own zk client to cluster.join (or cluster.connect), and their zk client has already established a session, then the cluster will register a watch for a SyncConnected event but never receive one, since it has already fired long ago, before the cluster started listening.
This bug was introduced in my earlier #13, which was a simple change that quietly violated the assumption that the cluster's zk client would trigger a SyncConnected after cluster.join.
This fix depends on (and includes) #14.