[client-v2] Fix check for ConnectTimeoutException in shouldRetry

[janeklb]

Summary

The catch block accepts ConnectTimeoutException; however, shouldRetry checks for ConnectException.

I think the solution is for shouldRetry to test for ConnectTimeoutException, please let me know if that's incorrect so that i can adjust the catch clause instead.

It's also worth noting that wrapException tests for ConnectException; however, wrapException will never only ever be called with NoHttpResponseException | ConnectionRequestTimeoutException | ConnectTimeoutException, which makes me wonder if:

• ConnectException there should be replaced with NoHttpResponseException
• or ConnectException should be added to the multi-catch

Checklist

Delete items not relevant to your PR:

• Unit and integration tests covering the common scenarios were added
• A human-readable description of the changes was provided to include in CHANGELOG
• For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

[chernser]

I think they should be both:

• ConnectException - means that socket failed to connect. When we implement TCP protocol support we might use this exception
• org.apache.hc.client5.http.ConnectTimeoutException is child of java.net.SocketTimeoutException - means that throws when read or accept (according to the JavaDoc of the last one). And ConnectTimeoutException is client specific exception. It will not be thrown by other client.

[janeklb]

ConnectException - means that socket failed to connect. When we implement TCP protocol support we might use this exception

I agree that this is a useful exception to check for, but this particular if block then proceeds to check retryCauses.contains(ClientFaultCause.ConnectTimeout), and so ConnectException doesn't seem applicable (to ClientFaultCause.ConnectTimeout)

[chernser]

It is applicable - because ClientFaultCause.ConnectTimeout is about any error while connection initiation.
ConnectException is all about it:

/**
 * Signals that an error occurred while attempting to connect a
 * socket to a remote address and port.  Typically, the connection
 * was refused remotely (e.g., no process is listening on the
 * remote address/port).
 *
 * @since   1.1
 */
public class ConnectException extends SocketException 

[chernser]

Good day, @janeklb !

You have brought a very good question and I almost agree with your fix.

ConnectionException should be in multi-catch.
ConnectionException is about connection and thrown by low-level socket, NoHttpResponseException happens when stale connection is used and Apache Http Client detects it.

When handling exceptions we are trying to be more generic and we try to avoid mentioning very specific exceptions. In this case both exceptions should be handles - unfortunately AHC 's ConnectTimeoutException doesn't derive from ConnectionException - I think this is wrong impl.

Please see my comment in the code.

[janeklb]

ConnectionException should be in multi-catch.

I've made this change in Client.java.

Do you think wrapException should be updated (to handle NoHttpResponseException)?

[janeklb]

Good day, @janeklb !

And good day to you too @chernser !

[chernser]

Do you think wrapException should be updated (to handle NoHttpResponseException)?
Lets do it - it is connection time exception in most cases.

[janeklb]

Alrighty. I've pushed the changes I think you were suggesting

[chernser]

Thank you!