test(showcase): add integration tests for retries - #13929
Conversation
… using SequenceService
There was a problem hiding this comment.
Code Review
This pull request introduces integration tests for verifying exponential backoff retries on both gRPC and HTTP/JSON clients using the SequenceServiceClient. It also adds corresponding helper methods in TestClientInitializer to initialize these clients with custom retry settings. The feedback suggests simplifying the helper methods in TestClientInitializer by configuring SequenceServiceSettings.Builder directly, which avoids intermediate stub settings and unnecessary object allocations.
…est/showcase-retries
…rviceSettings.Builder directly
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces integration tests for retry logic (ITRetries.java) across both gRPC and HTTP/JSON clients, covering scenarios such as exponential backoff, no retry, non-retryable errors, multiple status codes, and timeouts. It also adds helper methods in TestClientInitializer.java to initialize SequenceServiceClient instances with custom retry settings. The review feedback suggests improving code readability by removing redundant fully qualified class names (like com.google.rpc.Code and java.time.Duration) since these classes are already imported.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces integration tests for retry logic in ITRetries.java using both gRPC and HTTP/JSON clients, alongside helper methods in TestClientInitializer.java to initialize these clients with custom retry settings. The review feedback focuses on ensuring exception-safe resource management during client setup and teardown in the test class to prevent potential resource leaks, as well as removing redundant @SuppressWarnings("deprecation") annotations from the test methods.
| @BeforeAll | ||
| static void createClients() throws Exception { | ||
| grpcClient = TestClientInitializer.createGrpcSequenceClient(); | ||
| httpjsonClient = TestClientInitializer.createHttpJsonSequenceClient(); | ||
| } |
There was a problem hiding this comment.
If createHttpJsonSequenceClient() throws an exception during setup, grpcClient will be leaked because JUnit 5 does not execute @AfterAll methods if @BeforeAll fails. Consider wrapping the initialization in a try-catch block to close any successfully created clients if setup fails, ensuring exception-safe resource management.
| @BeforeAll | |
| static void createClients() throws Exception { | |
| grpcClient = TestClientInitializer.createGrpcSequenceClient(); | |
| httpjsonClient = TestClientInitializer.createHttpJsonSequenceClient(); | |
| } | |
| @BeforeAll | |
| static void createClients() throws Exception { | |
| try { | |
| grpcClient = TestClientInitializer.createGrpcSequenceClient(); | |
| httpjsonClient = TestClientInitializer.createHttpJsonSequenceClient(); | |
| } catch (Exception e) { | |
| if (grpcClient != null) { | |
| try { | |
| grpcClient.close(); | |
| } catch (Exception suppressed) { | |
| e.addSuppressed(suppressed); | |
| } | |
| } | |
| throw e; | |
| } | |
| } |
References
- When managing a collection of closeable resources, ensure they are closed in the reverse order of their creation (LIFO). The implementation must be exception-safe to prevent resource leaks, meaning all opened resources should be closed even if exceptions occur during their creation or closing.
| @AfterAll | ||
| static void destroyClients() throws InterruptedException { | ||
| grpcClient.close(); | ||
| httpjsonClient.close(); | ||
|
|
||
| grpcClient.awaitTermination(TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS); | ||
| httpjsonClient.awaitTermination( | ||
| TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS); | ||
| } |
There was a problem hiding this comment.
To ensure all clients are properly closed and terminated even if one of the operations throws an exception, use a robust try-finally structure to guarantee that each client's close() and awaitTermination() methods are called in reverse order of creation (LIFO).
@AfterAll
static void destroyClients() throws InterruptedException {
try {
if (grpcClient != null) {
grpcClient.close();
}
} finally {
try {
if (httpjsonClient != null) {
httpjsonClient.close();
}
} finally {
try {
if (grpcClient != null) {
grpcClient.awaitTermination(
TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS);
}
} finally {
if (httpjsonClient != null) {
httpjsonClient.awaitTermination(
TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS);
}
}
}
}
}References
- When managing a collection of closeable resources, ensure they are closed in the reverse order of their creation (LIFO). The implementation must be exception-safe to prevent resource leaks, meaning all opened resources should be closed even if exceptions occur during their creation or closing.
| @Test | ||
| @SuppressWarnings("deprecation") | ||
| void testGrpc_retryExponentialBackoff() throws Exception { |
There was a problem hiding this comment.
The @SuppressWarnings("deprecation") annotation is redundant on this test method (and all other test methods in this class) because they do not directly call any deprecated APIs. Removing these annotations keeps the code clean and prevents hiding actual deprecation warnings in the future.
@Test
void testGrpc_retryExponentialBackoff() throws Exception {
Description
This PR adds E2E retry integration tests verifying that client-side retry attempts adhere to the configured exponential backoff settings for both gRPC and HTTP/JSON transports.
Key Changes
TestClientInitializerfor creating theSequenceServiceClientconfigured with custom retry parameters and disabled jitter.SequenceServiceto record attempt timestamps on the server side, removing local CPU context-switching scheduling lag as a source of test flakiness.[UNAVAILABLE, UNAVAILABLE, UNAVAILABLE, OK]) and asserts that retry delays fall within the target exponential backoff timeline (with lenient safety margins).