[CURATOR-367] Delay reconnect on session expired #197
Conversation
| @@ -160,13 +162,33 @@ public void process(WatchedEvent event) | |||
| } | |||
| } | |||
|
|
|||
| // only wait during tests | |||
| assert waitOnExpiredEvent(event.getState()); | |||
There was a problem hiding this comment.
How is this meant to work? Are you assuming that assertions will only be enabled in testing? If you need some logic to only be executed during testing shouldn't there be some sort of internal flag indicating this?
There was a problem hiding this comment.
Yes, assuming assertions are enabled in testing. I've added the delay here wait for the reconnect to happen, but since now the reset is called later in the method it might no longer be necessary.
There was a problem hiding this comment.
I don't think that it's reasonable to assume that assertions will only be turned on during testing. If you look at something like LeaderSelector, it has specific code in there to support unit testing (the debugLeadershipLatch variable).
If you can cause the problem to occur without this code though, then it should be removed. I had a quick play with it and I couldn't seem to reproduce it without this code though.
There was a problem hiding this comment.
I changed using a debug flag instead of assert. I added an accessor class to be able to set the flag in the package private ConnectionState class.
I cannot cause the problem to occur without this code.
| } | ||
|
|
||
| // only for testing | ||
| private boolean waitOnExpiredEvent(Event.KeeperState currentState) |
There was a problem hiding this comment.
What is the purpose of the boolean return? The method only ever returns true? Is it purely so you can call it from the assert?
There was a problem hiding this comment.
Yes - only so that it can be called form assert.
|
Thanks for the update, looks OK now I think. I'll give it a bit more of a test early next week and then merge. |
|
Thanks for the review. I've also added the mockito dependency to the pom.xml, which I've left out of my previous commit. Since we're on Curator 2.x, we'd like to push this change to the 2.x version as well. Do I need to create another PR for that? |
|
I've run through some more tests and it all looks good. My only concern is that we're introducing the Mockito just for this test case. Could you move the test case into the curator-client sub package under org.apache.curator. Then no reflection would be required to set the debug flag and we could remove the mockito dependency? Do you have an opinion on introducing the Mockito dependency @Randgalt? In regards to the Curator 2.x and 3.x, don't worry about a separate PR, it should be ok to just merge from the Curator 2.x branch into 3.x. |
|
I'm -1 on introducing a new dependency. Can the PR be re-worked to not use Mockito? |
|
Thanks again for going through the change. I've put the test into curator-framework, because the org.apache.curator.framework.state.ConnectionStateManager got into an invalid state, that's why the CuratorFramework.blockUntilConnected didn't return and that's what I want to test. I'll remove the mockito dependency. |
|
This is a significant change. I would like amble time to look at it. @cammckenzie please do not merge. |
|
We need to see a version of this for the CURATOR-3.0 branch. The code has changed a lot there. |
|
I couldn't reproduce the issue with the CURATOR-3.0 branch. I think because ConnectionStateManager injects an Expired event after the session timeout has elapsed after the suspended event. |
|
Any thoughts on this @Randgalt , I'd like to merge before doing a build. |
| { | ||
| if (state == Event.KeeperState.Expired) | ||
| { | ||
| handleExpiredSession(); |
There was a problem hiding this comment.
We've lost functionality here right? If there's a new connection string and an expired session we don't get the new connection string.
There was a problem hiding this comment.
It gets handled in the handleState() method, which is called from process()
There was a problem hiding this comment.
It's handleState() that I'm concerned about. When state == Event.KeeperState.Expired the zooKeeper.hasNewConnectionString() isn't called.
There was a problem hiding this comment.
Isn't that true of the existing implementation though? If the state is expired then the checkNewConnectionString flag gets set to false so the handleNewConnectionString() method won't get called.
There was a problem hiding this comment.
Oh - I think you're right. The old version set checkNewConnectionString=false
No description provided.