Retries during handshake failure

[jdeppe-pivotal]

Similar to the situation as described here #1757. In this particular case I'm doing an append and occasionally get this failure when a server crashes:

io.lettuce.core.RedisException: io.netty.channel.unix.Errors$NativeIoException: readAddress(..) failed: Connection reset by peer
	at io.lettuce.core.internal.Exceptions.bubble(Exceptions.java:83)
	at io.lettuce.core.internal.Futures.awaitOrCancel(Futures.java:250)
	at io.lettuce.core.cluster.ClusterFutureSyncInvocationHandler.handleInvocation(ClusterFutureSyncInvocationHandler.java:130)
	at io.lettuce.core.internal.AbstractInvocationHandler.invoke(AbstractInvocationHandler.java:80)
	at com.sun.proxy.$Proxy32.append(Unknown Source)
	at org.apache.geode.redis.internal.executor.CrashAndNoRepeatDUnitTest.appendPerformAndVerify(CrashAndNoRepeatDUnitTest.java:269)
	at org.apache.geode.redis.internal.executor.CrashAndNoRepeatDUnitTest.lambda$givenServerCrashesDuringAPPEND_thenDataIsNotLost$1(CrashAndNoRepeatDUnitTest.java:115)
	at java.util.concurrent.CompletableFuture$AsyncRun.run$$$capture(CompletableFuture.java:1640)
	at java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

The append does not get retried.

Debugging this, I believe the error is coming from the RedisHandshakeHandler which is attempting to establish a connection before actually executing the append command. Should this step not also cause a retry of the command?

[mp911de]

I think what happens is that CommandHandler.exceptionCaught catches the exception and associates it with the head command in the command queue. With that, the command is completed and no longer retried.

TBCH, there seems no right or wrong in that regard. We could leave commands untouched. In return, commands could be retried once the connection is active again, but we lose that error propagation. A Connection reset by peer is at least somewhat expressive in the sense that the command might have been sent to the server but we do not know.

Happy to discuss other perspectives.

[jdeppe-pivotal]

Since I knew that commands should (and do) get retried on error, I was initially confused by this situation and it took me a while to debug it. The point of saying that, is that it was a surprise to me. Since establishing a connection is completely opaque to the user (and even more so in a clustered environment) it just seemed like the issued command had failed. In truth, the command hadn't even been sent though.

Ideally, I would prefer to have the library completely handle this situation. If I need to deal with this failure then I could just as well deal with (almost) any other failure and the benefits of having the library retry for me are greatly reduced.

Alternatively I would ask that this error condition be much more obvious to the user and produce an exception that more clearly indicates what went wrong.

[mp911de]

We have #1310 that touches on similar aspects as this discussion. I think that the docs do not explicitly mention exception behavior although we explain error handling for the various API's. There's quite a bit of docs at https://lettuce.io/core/release/reference/index.html#command-execution-reliability but I haven't found anything talking on the impact of network errors specifically.

I hadn't had the time yet to wrap my head fully around the retry bit mentioned with #1310. One thing to consider is that exceptions may be caused by a command itself and so retrying the command essentially would lead to an infinite loop of exceptions.

Alternatively I would ask that this error condition be much more obvious to the user and produce an exception that more clearly indicates what went wrong.

Do you have something specific on your mind?

[jdeppe-pivotal]

In terms of alternatives, I was just thinking of something more descriptive - for example a ConnectionHandshakeException that would wrap the lower level Netty exception.

[mp911de]

I see. At that stage, we do not have sufficient context to know that, we only know there was an exception and we have commands in the queue. Let me check whether we can do something about it.

[jdeppe-pivotal]

At a higher level, I am somewhat concerned that Lettuce will retry non-idempotent commands. I really like your suggestion in #1310 of allowing for a callback to decide whether the command should be retried. I think that would give the most control to the user and should be able to address the problem here too.