DefaultRawMemcacheClient logs error but does now throw

[jvaleo]

Whenever there is a timeout an error is logged but there is not exception thrown. This means that the caller can't catch anything and do something else. The use case for us is to catch a timeout exception, throw a custom timeout exception our framework provides which ticks metrics, logs, et cetera. This is what we do for all clients we use (Cassandra, RPC, et cetera).

If you're OK with this change, we can submit a patch for this behavior.

https://github.com/spotify/folsom/blob/master/src/main/java/com/spotify/folsom/client/DefaultRawMemcacheClient.java#L285

[jvaleo]

(Ping)

[danielnorberg]

@jvaleo Looks like all outstanding request futures will be failed when the channel is closed on timeout:

https://github.com/spotify/folsom/blob/master/src/main/java/com/spotify/folsom/client/DefaultRawMemcacheClient.java#L319

Are you seeing futures not failing on timeout?

[jvaleo]

@danielnorberg So the issue is that an error is logged every time the client times out, but it's difficult to take an action on those timeouts (vs another exception) because all client exceptions cause outstanding futures to throw the same exception type (with a different message). We want to monitor or take a specific action on a timeout and its not ideal to have to catch the MemcacheClosedException and check for the string "Timeout". Ideally, the exception thrown by the future would extend TimeoutException (or the future itself could timeout) so that we could manage them cleanly.

[danielnorberg]

@jvaleo Right, that sounds useful.

[jvaleo]

@danielnorberg Is that something we can just PR?

[protocol7]

@jvaleo Feel free to push a PR and we'll happily review it

[mniebla]

Something I was interested too...so I took a crack at it, here you go: #71