jbonofre commented on code in PR #10140:
URL: https://github.com/apache/iceberg/pull/10140#discussion_r1587385129
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcClientPool.java:
##########
@@ -43,8 +65,18 @@ public JdbcClientPool(String dbUrl, Map<String, String>
props) {
}
public JdbcClientPool(int poolSize, String dbUrl, Map<String, String> props)
{
- super(poolSize, SQLNonTransientConnectionException.class, true);
+ super(poolSize, SQLTransientException.class, true);
properties = props;
+ retryableStatusCodes = Sets.newHashSet();
+ retryableStatusCodes.addAll(COMMON_RETRYABLE_CONNECTION_SQL_STATES);
+ String configuredRetryableStatuses =
props.get(JdbcUtil.RETRYABLE_STATUS_CODES);
Review Comment:
Thanks to make it configuration (as discussed in the PR): it will allow
users to define database specific states.
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcClientPool.java:
##########
@@ -21,17 +21,39 @@
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;
-import java.sql.SQLNonTransientConnectionException;
+import java.sql.SQLTransientException;
+import java.util.Arrays;
import java.util.Map;
import java.util.Properties;
+import java.util.Set;
+import java.util.stream.Collectors;
import org.apache.iceberg.CatalogProperties;
import org.apache.iceberg.ClientPoolImpl;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
public class JdbcClientPool extends ClientPoolImpl<Connection, SQLException> {
+ /**
+ * The following are common retryable SQLSTATE error codes which are generic
across vendors.
+ *
+ * <ul>
+ * <li>08000: Generic Connection Exception
+ * <li>08003: Connection does not exist
+ * <li>08006: Connection failure
+ * <li>08007: Transaction resolution unknown
+ * </ul>
+ *
+ * See https://en.wikipedia.org/wiki/SQLSTATE for more details.
+ */
+ static final Set<String> COMMON_RETRYABLE_CONNECTION_SQL_STATES =
Review Comment:
Maybe worth to add `40001` and `40P01` state to the list:
- `40001` is the code used by most databases to identify deadlocks
- `40P01` is a PostgreSQL specific code for deadlocks as well (see
https://www.postgresql.org/docs/13/errcodes-appendix.html)
Deadlocks are a candidate to retry imho.
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java:
##########
@@ -40,6 +40,8 @@ final class JdbcUtil {
// property to control if view support is added to the existing database
static final String SCHEMA_VERSION_PROPERTY = JdbcCatalog.PROPERTY_PREFIX +
"schema-version";
+ static final String RETRYABLE_STATUS_CODES = "retryable_status_codes";
Review Comment:
My take on this (maybe I'm wrong) is that anything specific to JDBC catalog
should be prefixed by `jdbc`, just to be sure we don't have conflict with
properties with same name not related to JDBC Catalog.
As JDBC Connection is used in JDBC Catalog, I would prefix with `jdbc` for
consistency. As it's user facing properties, it's important to be consistent
here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
