JdkHttpSender should retry on connect exceptions #5867
Conversation
Codecov ReportAttention:
... and 38 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
| new JdkHttpSender( | ||
| mockHttpClient, | ||
| "http://10.255.255.1", // Connecting to a non-routable IP address to trigger connection | ||
| // timeout | ||
| false, | ||
| "text/plain", | ||
| Duration.ofSeconds(10).toNanos(), | ||
| Collections::emptyMap, | ||
| RetryPolicy.builder() | ||
| .setMaxAttempts(2) | ||
| .setInitialBackoff(Duration.ofMillis(1)) | ||
| .build()); |
There was a problem hiding this comment.
For anther PR of course, but maybe the JdkHttpSender is ready for a builder. :)
| throw new IllegalStateException(e); | ||
| } finally { | ||
| byteBufferPool.resetPool(); | ||
| } | ||
| } | ||
|
|
||
| private static boolean isRetryableException(IOException throwable) { | ||
| return throwable instanceof HttpConnectTimeoutException; |
There was a problem hiding this comment.
Have we thought about expanding the scope a bit to include the parent type HttpTimeoutException? This could allow for retries for send timeouts beyond just the connection timeout.
There was a problem hiding this comment.
So for OkHttp, there are settings for connectTimeout, readTimeout, and writeTimeout. When these settings are exceeded, you get a corresponding timeout.
For JDK HttpClient, there's only a connectTimeout setting. So I'm not actually sure what the read / write timeout settings are or how to reproduce one of these things in tests to ensure we're doing the right thing.
With that said, I do think we want to retry in all timeout scenarios, so I'm going to expand the criteria of retryable exceptions to include any HttpTimeoutException.
… HttpTimeoutException
Fixes a TODO in JdkHttpSender by ensuring that it retries on connect exceptions.
Thanks to @mikelaspina for the report!