Add retry support for HTTP execution failure#1702
Conversation
balamurugana
force-pushed
the
Add-retry-support-for-HTTP-execution-failure
branch
8 times, most recently
from
5475995 to
81ad96c
Compare
…cts() cleanup (minio#1700)" This reverts commit 63242f0.
Signed-off-by: Bala.FA <bala@minio.io>
balamurugana
force-pushed
the
Add-retry-support-for-HTTP-execution-failure
branch
from
81ad96c to
9e8f51f
Compare
| 408, // Request Timeout | ||
| 429, // Too Many Requests | ||
| 499, // Client Closed Request (nginx) | ||
| 500, // Internal Server Error | ||
| 502, // Bad Gateway | ||
| 503, // Service Unavailable | ||
| 504, // Gateway Timeout | ||
| 520); // Cloudflare unknown error |
There was a problem hiding this comment.
should we use http.StatusTooManyRequests kind values instead of hardcoded ones like 429?
There was a problem hiding this comment.
okhttp does not have those enums? java.net.HttpURLConnection has limited constants. I think we can avoid mixed usage.
allanrogerr
left a comment
allanrogerr
left a comment
There was a problem hiding this comment.
The new retry logic including interceptor, setRetry replacement logic, body-replay across retries, NPE-safe trace formatting are not unit-tested. The functional tests cover a happy path, but
- no test creates a scenario to verify retry actually fires,
- no test verifies the maxRetries boundary,
- no test checks setRetry replaces the interceptor correctly,
- no test exercises a malformed request.method()
Could you have Claude generate some tests please?
| .replaceAll("Signature=([0-9a-f]+)", "Signature=*REDACTED*") | ||
| .replaceAll("Credential=([^/]+)", "Credential=*REDACTED*")); |
There was a problem hiding this comment.
These could be stored as static final Pattern to avoid recompilation every trace event
| int i = 0; | ||
| boolean found = false; | ||
| for (i = 0; i < interceptors.size(); i++) { |
There was a problem hiding this comment.
Any reason for re-reinitialising?
| if (traceStream == null) return this.httpClient; | ||
|
|
||
| OkHttpClient httpClient = this.httpClient; | ||
| List<Interceptor> interceptors = httpClient.interceptors(); |
There was a problem hiding this comment.
This look very close to the code on line 151. Any reason for the duplication? This could be refactored out as a method.
| * Sets request retry parameters. Any null/invalid values disable retry. | ||
| * | ||
| * <pre>Example:{@code | ||
| * minioClient.setRetryParameters(ImmutableSet.of(408, 504), 250, 3); |
There was a problem hiding this comment.
| * minioClient.setRetryParameters(ImmutableSet.of(408, 504), 250, 3); | |
| * minioClient.setRetry(ImmutableSet.of(408, 504), 250, 3); |
allanrogerr
left a comment
allanrogerr
left a comment
There was a problem hiding this comment.
Additional comments @balamurugana PTAL
| boolean statusRetryInterceptorFound = true; | ||
|
|
||
| OkHttpClient httpClient = this.httpClient; | ||
| // FIXME: enable retry for all request. | ||
| // if (!s3request.retryFailure()) { | ||
| // httpClient = httpClient.newBuilder().retryOnConnectionFailure(false).build(); | ||
| // } | ||
| StringBuilder traceBuilder = new StringBuilder(request.httpTraces()); | ||
| if (!statusRetryInterceptorFound && traceStream != null) { | ||
| traceStream.print(request.httpTraces()); | ||
| } |
There was a problem hiding this comment.
Why not static final if it is never reassigned? Also all branches using this boolean are unreachable. Perhaps there is an unfinished idea here?
| builder.addInterceptor(interceptor); | ||
| } | ||
|
|
||
| this.httpClient = builder.build(); |
There was a problem hiding this comment.
This seems to have the makings of a race:
- read: List interceptors = this.httpClient.interceptors();
- read: OkHttpClient.Builder builder = this.httpClient.newBuilder();
- write: this.httpClient = builder.build();
Two concurrent setRetry calls can read and assign protected volatile OkHttpClient httpClient. This may not be ideal.
| try { | ||
| return Method.valueOf(value.toUpperCase(Locale.US).trim()); | ||
| } catch (IllegalArgumentException e) { | ||
| return null; |
There was a problem hiding this comment.
Invocations of the method may hit NPE - e.g. traceBuilder.append(method.toString())
3 participants
No description provided.