This is an automated email from the ASF dual-hosted git repository.
etudenhoefner pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg.git
The following commit(s) were added to refs/heads/main by this push:
new 26048839cb Core: Fix a cast that is too narrow (#12743)
26048839cb is described below
commit 26048839cb795a1e2c38ed592b45021fe37fdd51
Author: Angelo Genovese <[email protected]>
AuthorDate: Fri Jul 11 02:10:26 2025 -0400
Core: Fix a cast that is too narrow (#12743)
The current cast assumes that the type of the exception matches the type of
reconnectExc,
in the JdbcClientPool the isConnectionException method can return true for
any SQLException
with the correct sql status. The exception is always a subclass of the
parameterized type though.
---
.../java/org/apache/iceberg/ClientPoolImpl.java | 2 +-
.../org/apache/iceberg/TestClientPoolImpl.java | 101 ++++++++++++++++++++-
2 files changed, 97 insertions(+), 6 deletions(-)
diff --git a/core/src/main/java/org/apache/iceberg/ClientPoolImpl.java
b/core/src/main/java/org/apache/iceberg/ClientPoolImpl.java
index f454c4aeea..e0e42b8ae8 100644
--- a/core/src/main/java/org/apache/iceberg/ClientPoolImpl.java
+++ b/core/src/main/java/org/apache/iceberg/ClientPoolImpl.java
@@ -82,7 +82,7 @@ public abstract class ClientPoolImpl<C, E extends Exception>
retryAttempts++;
Thread.sleep(CONNECTION_RETRY_WAIT_PERIOD_MS);
} else {
- throw reconnectExc.cast(exc);
+ throw (E) exc;
}
}
}
diff --git a/core/src/test/java/org/apache/iceberg/TestClientPoolImpl.java
b/core/src/test/java/org/apache/iceberg/TestClientPoolImpl.java
index 8204c8640a..7b33121af7 100644
--- a/core/src/test/java/org/apache/iceberg/TestClientPoolImpl.java
+++ b/core/src/test/java/org/apache/iceberg/TestClientPoolImpl.java
@@ -21,6 +21,8 @@ package org.apache.iceberg;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import java.util.List;
+import java.util.function.Supplier;
import org.junit.jupiter.api.Test;
public class TestClientPoolImpl {
@@ -40,7 +42,28 @@ public class TestClientPoolImpl {
.as("There should be exactly one successful action invocation")
.isEqualTo(1);
assertThat(mockClientPool.reconnectionAttempts()).isEqualTo(succeedAfterAttempts
- 1);
-
assertThat(mockClientPool.clients().peekFirst().equals(firstClient)).isFalse();
+ assertThat(mockClientPool.clients()).first().isNotEqualTo(firstClient);
+ }
+ }
+
+ @Test
+ public void customExceptionIsRetried() throws Exception {
+ int maxRetries = 5;
+ int succeedAfterAttempts = 3;
+ try (MockClientPoolImpl mockClientPool =
+ new MockClientPoolImpl(2, RetryableException.class, true, maxRetries))
{
+ // initialize the client pool with a client, so that we can verify the
client is replaced
+ MockClient firstClient = mockClientPool.newClient();
+ mockClientPool.clients().add(firstClient);
+
+ int actions =
+ mockClientPool.run(
+ client -> client.succeedAfter(succeedAfterAttempts, () -> new
CustomException(true)));
+ assertThat(actions)
+ .as("There should be exactly one successful action invocation")
+ .isEqualTo(1);
+
assertThat(mockClientPool.reconnectionAttempts()).isEqualTo(succeedAfterAttempts
- 1);
+ assertThat(mockClientPool.clients()).first().isNotEqualTo(firstClient);
}
}
@@ -58,11 +81,42 @@ public class TestClientPoolImpl {
}
}
+ @Test
+ public void nonRetryableExceptionAfterRetryableException() {
+ try (MockClientPoolImpl mockClientPool =
+ new MockClientPoolImpl(2, RetryableException.class, true, 3)) {
+ assertThatThrownBy(
+ () ->
+ mockClientPool.run(
+ client ->
+ client.succeedAfter(
+ List.of(
+ new CustomException(true),
+ new CustomException(true),
+ new CustomException(false)))))
+ .isInstanceOf(CustomException.class)
+ .hasMessage(null);
+ assertThat(mockClientPool.reconnectionAttempts()).isEqualTo(2);
+ }
+ }
+
@Test
public void testNoRetryingNonRetryableException() {
try (MockClientPoolImpl mockClientPool =
new MockClientPoolImpl(2, RetryableException.class, true, 3)) {
- assertThatThrownBy(() ->
mockClientPool.run(MockClient::failWithNonRetryable, true))
+ assertThatThrownBy(() ->
mockClientPool.run(MockClient::throwNonRetryableException, true))
+ .isInstanceOf(NonRetryableException.class)
+ .hasMessage(null);
+ assertThat(mockClientPool.reconnectionAttempts()).isEqualTo(0);
+ }
+ }
+
+ @Test
+ public void customNonRetryableExceptionIsNotRetried() {
+ try (MockClientPoolImpl mockClientPool =
+ new MockClientPoolImpl(2, RetryableException.class, true, 3)) {
+ assertThatThrownBy(
+ () ->
mockClientPool.run(MockClient::throwCustomNonRetryableException, true))
.isInstanceOf(NonRetryableException.class)
.hasMessage(null);
assertThat(mockClientPool.reconnectionAttempts()).isEqualTo(0);
@@ -84,6 +138,18 @@ public class TestClientPoolImpl {
static class NonRetryableException extends RuntimeException {}
+ static class CustomException extends NonRetryableException {
+ private final boolean retryable;
+
+ CustomException(boolean retryable) {
+ this.retryable = retryable;
+ }
+
+ public boolean isRetryable() {
+ return retryable;
+ }
+ }
+
static class MockClient {
boolean closed = false;
int actions = 0;
@@ -104,18 +170,37 @@ public class TestClientPoolImpl {
return actions;
}
- int succeedAfter(int succeedAfterAttempts) {
+ int succeedAfter(List<RuntimeException> exceptions) {
+ int succeedAfterAttempts = exceptions.size();
+ if (retryableFailures == succeedAfterAttempts) {
+ return successfulAction();
+ }
+
+ RuntimeException runtimeException = exceptions.get(retryableFailures);
+ retryableFailures++;
+ throw runtimeException;
+ }
+
+ int succeedAfter(int succeedAfterAttempts, Supplier<RuntimeException>
exceptionSupplier) {
if (retryableFailures == succeedAfterAttempts - 1) {
return successfulAction();
}
retryableFailures++;
- throw new RetryableException();
+ throw exceptionSupplier.get();
}
- int failWithNonRetryable() {
+ int succeedAfter(int succeedAfterAttempts) {
+ return succeedAfter(succeedAfterAttempts, RetryableException::new);
+ }
+
+ int throwNonRetryableException() {
throw new NonRetryableException();
}
+
+ int throwCustomNonRetryableException() {
+ throw new CustomException(false);
+ }
}
static class MockClientPoolImpl extends ClientPoolImpl<MockClient,
Exception> {
@@ -146,6 +231,12 @@ public class TestClientPoolImpl {
client.close();
}
+ @Override
+ protected boolean isConnectionException(Exception exc) {
+ return super.isConnectionException(exc)
+ || (exc instanceof CustomException && ((CustomException)
exc).isRetryable());
+ }
+
int reconnectionAttempts() {
return reconnectionAttempts;
}
