fix zk session leak #678
fix zk session leak #678
Conversation
…ting a new one (linkedin#671)" This reverts commit c3bd17b.
ahmedahamid
requested review from
srinipunuru,
akaritakai,
somandal and
celiakung
| @@ -201,9 +207,15 @@ public void disconnect() { | |||
| * the actions that need to be taken with them, which are implemented in the Coordinator class | |||
| */ | |||
| public void connect() { | |||
| disconnect(); // Guard against leaking an existing zookeeper session | |||
There was a problem hiding this comment.
Removing this will not be sufficient. On Zookeeper session expiry, the live instance ephemeral node will be deleted. So, the leader will become follower and both needs to creates new live instance nodes. Once the role will change, all the listener cleanup will be required which is happening in disconnect.
There was a problem hiding this comment.
Thanks for the review @vmaheshw.
It seems like you are correct. I might be leaking subscriptions. I may not have experienced any issues as the subscriptions are to the nodes that would never get updated.
I will add the clean up logic as necessary.
There was a problem hiding this comment.
Why remove this line anyway? If we are connecting or reconnecting, then we should not have an existing session. The guard code defensively ensures that any existing connection is definitely closed.
| public class ZkStateChangeListener implements IZkStateListener { | ||
| @Override | ||
| public void handleStateChanged(Watcher.Event.KeeperState state) { | ||
| } |
There was a problem hiding this comment.
In the situation, when the session never gets reestablished, handleNewSession will not get invoked. So, handleStateChanged with state EXPIRY needs to be handled.
There was a problem hiding this comment.
Noted. Will post a new commit. Thanks
This PR has a bug fix to a specific problem where the cluster could eventually go leaderless as leadership transitions occur. It handles the session re-establishment properly and avoid session leaks.