Skip to content

Commit 756e294

Browse files
committed
docs(retry): clarify RetryInterceptor as supported API; add terminal-status assertions
Address Copilot review feedback (PR #1701, review 4246873092): - RetryInterceptor Javadoc now declares it as part of the SDK's supported public API and documents how BaseS3Client wraps every supplied OkHttpClient. Drops {@link} references to package-private Retry helpers so the public Javadoc no longer points at internal symbols. Adds a Threading note about Thread.sleep blocking the OkHttp dispatcher slot during backoff and points high-concurrency callers at Dispatcher pool sizing. - BaseS3Client.executeAsync Javadoc reworded to reflect the wrapWithRetry behaviour (interceptor installed on every supplied client, default or caller-provided) and to note the maxRetries supplier is read per request. - RetryTest: strengthen testRetryExhaustedReturnsLastResponse to assert the InvalidResponseException reflects HTTP 500 rather than allowing any exception type to pass. Add testRetryExhaustedSurfacesXmlErrorResponse for the XML/ErrorResponseException path, asserting both response.code() and the parsed S3 code after retries are exhausted. Validated locally on Java 17: - :api:check (compile + spotless + spotbugs + tests) PASS - :api:javadoc PASS
1 parent 6d39a12 commit 756e294

3 files changed

Lines changed: 54 additions & 15 deletions

File tree

api/src/main/java/io/minio/BaseS3Client.java

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -297,8 +297,11 @@ private String[] handleRedirectResponse(
297297
}
298298

299299
/**
300-
* Execute HTTP request asynchronously for given parameters. Retries on retryable HTTP status
301-
* codes are handled by {@link Http.RetryInterceptor} installed on the default HTTP client.
300+
* Execute HTTP request asynchronously for given parameters. Retries on retryable IOException,
301+
* HTTP status, and S3 error code are handled by {@link Http.RetryInterceptor}, which {@link
302+
* #wrapWithRetry} installs on the underlying {@link OkHttpClient} regardless of whether the
303+
* client is the default or caller-supplied. The attempt budget is read from {@link #maxRetries}
304+
* on every request.
302305
*/
303306
protected CompletableFuture<Response> executeAsync(Http.S3Request s3request, String region) {
304307
Credentials credentials = (provider == null) ? null : provider.fetch();

api/src/main/java/io/minio/Http.java

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -692,24 +692,32 @@ public static OkHttpClient setTimeout(
692692
/**
693693
* OkHttp interceptor that retries transient HTTP failures with full-jitter exponential backoff.
694694
*
695+
* <p>This is part of the SDK's supported public API: it is installed automatically by {@link
696+
* BaseS3Client} on every supplied {@link OkHttpClient} (default or caller-provided), and may also
697+
* be registered explicitly on a stand-alone {@link OkHttpClient} via {@code .addInterceptor(new
698+
* Http.RetryInterceptor())}.
699+
*
695700
* <p>Retries on:
696701
*
697702
* <ul>
698-
* <li>retryable IOException ({@link Retry#isRequestErrorRetryable}) — connection reset, EOF,
699-
* socket timeout, idle-connection close. Excludes TLS handshake / unknown-CA / HTTPS
700-
* protocol mismatch.
701-
* <li>retryable HTTP status code ({@link Retry#RETRYABLE_HTTP_STATUS_CODES}) — 408, 429, 499,
702-
* 500, 502, 503, 504, 520.
703-
* <li>retryable S3 error code in a non-2xx response body ({@link Retry#RETRYABLE_S3_CODES}) —
704-
* {@code SlowDown}, {@code InternalError}, {@code ExpiredToken}, etc.
703+
* <li>retryable IOException — connection reset, EOF, socket timeout, idle-connection close.
704+
* Excludes TLS handshake / unknown-CA / HTTPS protocol mismatch.
705+
* <li>retryable HTTP status code — 408, 429, 499, 500, 502, 503, 504, 520.
706+
* <li>retryable S3 error code in a non-2xx response body — {@code SlowDown}, {@code
707+
* InternalError}, {@code ExpiredToken}, etc.
705708
* </ul>
706709
*
707-
* <p>Backoff is computed by {@link Retry#exponentialBackoffMs}. The maximum number of attempts is
708-
* supplied per intercept call via {@link IntSupplier} so that an SDK client can expose runtime
709-
* tuning while keeping the interceptor itself stateless. The no-arg constructor uses the default
710-
* {@link Retry#MAX_RETRY}.
710+
* <p>Backoff is full-jitter exponential, with a 200&nbsp;ms unit and a 1&nbsp;s per-attempt cap.
711+
* The maximum number of attempts is supplied per intercept call via {@link IntSupplier} so that
712+
* an SDK client can expose runtime tuning while keeping the interceptor itself stateless. The
713+
* no-arg constructor uses the package default of 10 attempts.
714+
*
715+
* <p><b>Threading.</b> Backoff sleeps on the OkHttp dispatcher thread that owns the call. Under
716+
* sustained 5xx/429 storms this can hold dispatcher slots idle while waiting. Callers that need
717+
* higher concurrency under widespread retry should size the dispatcher worker pool accordingly
718+
* (see {@link okhttp3.Dispatcher#setMaxRequests} / {@code setMaxRequestsPerHost}).
711719
*
712-
* <p>To opt out of retries on a custom {@link OkHttpClient}, simply do not register this
720+
* <p>To opt out of retries on a stand-alone {@link OkHttpClient}, simply do not register this
713721
* interceptor.
714722
*/
715723
public static class RetryInterceptor implements Interceptor {

api/src/test/java/io/minio/RetryTest.java

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,35 @@ public void testRetryExhaustedReturnsLastResponse() throws IOException, MinioExc
306306
client.listBuckets();
307307
Assert.fail("expected exception after exhausted retries");
308308
} catch (InvalidResponseException e) {
309-
// expected — non-XML 500
309+
// Terminal exception must surface the underlying 500 status so callers can distinguish
310+
// exhausted retries from unrelated failures (e.g. NPE, parser bugs).
311+
Assert.assertTrue(
312+
"exhausted-retries exception must reflect HTTP 500, got: " + e.getMessage(),
313+
e.getMessage().contains("Response code: 500"));
314+
}
315+
Assert.assertEquals(3, server.getRequestCount());
316+
}
317+
}
318+
319+
@Test
320+
public void testRetryExhaustedSurfacesXmlErrorResponse() throws IOException, MinioException {
321+
// 3 attempts, all 503 with XML <Error><Code>InternalError</Code> — confirms the terminal
322+
// ErrorResponseException carries both the 503 status and the parsed S3 code after the retry
323+
// loop gives up.
324+
try (MockWebServer server = new MockWebServer()) {
325+
server.enqueue(xmlError(503, "InternalError"));
326+
server.enqueue(xmlError(503, "InternalError"));
327+
server.enqueue(xmlError(503, "InternalError"));
328+
server.start();
329+
330+
MinioClient client =
331+
MinioClient.builder().endpoint(server.url("").toString()).maxRetries(3).build();
332+
try {
333+
client.listBuckets();
334+
Assert.fail("expected ErrorResponseException after exhausted retries");
335+
} catch (ErrorResponseException e) {
336+
Assert.assertEquals(503, e.response().code());
337+
Assert.assertEquals("InternalError", e.errorResponse().code());
310338
}
311339
Assert.assertEquals(3, server.getRequestCount());
312340
}

0 commit comments

Comments
 (0)