These are chat archives for atomix/atomix

16th
Jun 2016
Jordan Halterman
@kuujo
Jun 16 2016 00:09
Actually, it seems like some behavior may have changed here. The ResourceRecoveryTests are no longer passing, and this line is returning an exception
SessionClosedException to be specific, when it should be opening a new session
Madan Jampani
@madjam
Jun 16 2016 00:10
Let me take a look
Madan Jampani
@madjam
Jun 16 2016 00:43
@kuujo are you also seeing the map.get()call timeout?
Jordan Halterman
@kuujo
Jun 16 2016 00:50
Haven't looked hard enough... Just got home and gonna take a closer look
Jordan Halterman
@kuujo
Jun 16 2016 01:14
looks like I broke it...
Madan Jampani
@madjam
Jun 16 2016 01:16
how so?
Jordan Halterman
@kuujo
Jun 16 2016 01:16
actually maybe not… seems like the recover code is correct
recover() completes successfully but the session is still closed
I may have a fix for it
Madan Jampani
@madjam
Jun 16 2016 01:19
What I’m seeing is that it closes the old session (that existed before recover was called) and successfully opens a new one
Jordan Halterman
@kuujo
Jun 16 2016 01:23
@madjam what I’m seeing is when it gets to this line, the client’s new session is still closed with a 0 session ID: https://github.com/atomix/copycat/blob/master/client/src/main/java/io/atomix/copycat/client/DefaultCopycatClient.java#L254
Madan Jampani
@madjam
Jun 16 2016 01:24
Hmmm. Looks like recover() is completing before the new session is registered.
Jordan Halterman
@kuujo
Jun 16 2016 01:24
right
maybe something odd about the method chaining with handleAsync
gonna check how far register actually gets
yeah that’s definitely completing before register
Madan Jampani
@madjam
Jun 16 2016 01:27
I think the problem is in line 248
Jordan Halterman
@kuujo
Jun 16 2016 01:28
Well… handleAsync is suppose to return a future that, when completed, completes the stage… I’m just going to change it to nest both of them to be safe
Madan Jampani
@madjam
Jun 16 2016 01:29
the first handleAsync is returning a CompletableFuture<CompletableFuture<Session>>
May be your intention was to return a CompletableFuture<Session>?
that completes when the session is registered?
Jordan Halterman
@kuujo
Jun 16 2016 01:30
ahh
      recoverFuture = new CompletableFuture<>();
      session.close().whenCompleteAsync((closeResult, closeError) -> {
        session = newSession();
        session.register().whenCompleteAsync((registerResult, registerError) -> {
          CompletableFuture<CopycatClient> recoverFuture = this.recoverFuture;
          if (registerError == null) {
            recoverFuture.complete(this);
          } else {
            recoverFuture.completeExceptionally(registerError);
          }
          this.recoverFuture = null;
        }, eventContext.executor());
      }, eventContext.executor());
Madan Jampani
@madjam
Jun 16 2016 01:31
This looks better.
Jordan Halterman
@kuujo
Jun 16 2016 01:31
just going to do that because I know it will work :-)
one sec I’ll test it
Madan Jampani
@madjam
Jun 16 2016 01:32
that fixes it :)
Jordan Halterman
@kuujo
Jun 16 2016 01:32
yeah
k try again
running the Atomix tests fully then I’ll release them
Madan Jampani
@madjam
Jun 16 2016 01:37
cool
Jordan Halterman
@kuujo
Jun 16 2016 01:54
lookin good
Jordan Halterman
@kuujo
Jun 16 2016 02:07
I am getting a timeout exception too
in that test… during shutdown
while closing the client
Jordan Halterman
@kuujo
Jun 16 2016 02:20
while closing resources that belong to the client...
(working through the code)
hmm… InstanceClient is still marked CLOSED when the tests end
Jordan Halterman
@kuujo
Jun 16 2016 02:25
state changes in the client just seem to be behaving a little differently
almost have it figured out
think I got it...
Jordan Halterman
@kuujo
Jun 16 2016 02:33
So, basically in the test calling recover() was causing the original session to transition to CLOSED when it would normally be EXPIRED if you’re calling recover(). The InstanceClient has some logic to assume CLOSED means someone actually called close(), which isn’t the case in the test. Creating the new session before closing the old session solves that problem, but I need to poke around a bit and make sure the right state changes are still happening. Maybe should just make the test behave like it should in practice - calling recover() on a client with an EXPIRED session.
Madan Jampani
@madjam
Jun 16 2016 02:34
That might be a good idea: recover a EXPIRED session.
Jordan Halterman
@kuujo
Jun 16 2016 02:35
yeah… I think I’m going to add an expire method to ClientSession for testing
that test shouldn’t really be calling recover() but should just force the expiration of the session and let it recover
Madan Jampani
@madjam
Jun 16 2016 02:37
So does expire merely shutdown keepalive task?
Jordan Halterman
@kuujo
Jun 16 2016 03:32
hmm
shut down the keep-alive and set the state to EXPIRED
Madan Jampani
@madjam
Jun 16 2016 03:44
yep. that should do it.
Jordan Halterman
@kuujo
Jun 16 2016 03:44
k submitting a couple PRs
those two should fix our issues… running the tests again
Jordan Halterman
@kuujo
Jun 16 2016 03:59
k now the tests are all passing and pushing release
Jordan Halterman
@kuujo
Jun 16 2016 04:46
@madjam all released
Madan Jampani
@madjam
Jun 16 2016 04:47
thanks @kuujo!
Jordan Halterman
@kuujo
Jun 16 2016 04:55
fixed some really good bugs too :-) thanks!