dimas-b commented on code in PR #1517:
URL: https://github.com/apache/polaris/pull/1517#discussion_r2074317542
##########
extension/persistence/relational-jdbc/src/test/java/org/apache/polaris/extension/persistence/impl/relational/jdbc/AtomicMetastoreManagerWithJdbcBasePersistenceImplTest.java:
##########
@@ -67,4 +70,22 @@ protected PolarisTestMetaStoreManager
createPolarisTestMetaStoreManager() {
new PolarisConfigurationStore() {},
timeSource.withZone(ZoneId.systemDefault())));
}
+
+ private static class H2JdbcConfiguration implements
RelationalJdbcConfiguration {
Review Comment:
nit: use `@PolarisImmutable` with default values.
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -173,23 +190,82 @@ public int executeUpdate(String query) throws
SQLException {
* @throws SQLException : Exception caught during transaction execution.
*/
public void runWithinTransaction(TransactionCallback callback) throws
SQLException {
- try (Connection connection = borrowConnection()) {
- boolean autoCommit = connection.getAutoCommit();
- connection.setAutoCommit(false);
- boolean success = false;
+ withRetries(
+ () -> {
+ try (Connection connection = borrowConnection()) {
+ boolean autoCommit = connection.getAutoCommit();
+ boolean success = false;
+ connection.setAutoCommit(false);
+ try {
+ try (Statement statement = connection.createStatement()) {
+ success = callback.execute(statement);
+ }
+ } finally {
+ if (success) {
+ connection.commit();
+ } else {
+ connection.rollback();
+ }
+ connection.setAutoCommit(autoCommit);
+ }
+ }
+ return null;
+ });
+ }
+
+ private boolean isRetryable(SQLException e) {
+ String sqlState = e.getSQLState();
+
+ if (sqlState != null) {
+ return sqlState.equals(DEADLOCK_SQL_CODE)
+ || // Deadlock detected
+ sqlState.equals(SERIALIZATION_FAILURE_SQL_CODE); // Serialization
failure
+ }
+
+ // Additionally, one might check for specific error messages or other
conditions
+ return e.getMessage().contains("connection refused")
+ || e.getMessage().contains("connection reset");
+ }
+
+ public <T> T withRetries(Operation<T> operation) throws SQLException {
+ int attempts = 0;
+ // maximum number of retries.
+ int maxAttempts = relationalJdbcConfiguration.maxRetries().orElse(1);
+ // How long we should try, since the first attempt.
+ long maxDuration =
relationalJdbcConfiguration.maxDurationInMs().orElse(100L);
Review Comment:
I'd go with a higher default, like 5000 ms. I believe defaults should be
lenient to allow the highest success rate. Users can tune them down if they
prefer faster response times to higher success rates.
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -173,23 +190,82 @@ public int executeUpdate(String query) throws
SQLException {
* @throws SQLException : Exception caught during transaction execution.
*/
public void runWithinTransaction(TransactionCallback callback) throws
SQLException {
- try (Connection connection = borrowConnection()) {
- boolean autoCommit = connection.getAutoCommit();
- connection.setAutoCommit(false);
- boolean success = false;
+ withRetries(
+ () -> {
+ try (Connection connection = borrowConnection()) {
+ boolean autoCommit = connection.getAutoCommit();
+ boolean success = false;
+ connection.setAutoCommit(false);
+ try {
+ try (Statement statement = connection.createStatement()) {
+ success = callback.execute(statement);
+ }
+ } finally {
+ if (success) {
+ connection.commit();
+ } else {
+ connection.rollback();
+ }
+ connection.setAutoCommit(autoCommit);
+ }
+ }
+ return null;
+ });
+ }
+
+ private boolean isRetryable(SQLException e) {
+ String sqlState = e.getSQLState();
+
+ if (sqlState != null) {
+ return sqlState.equals(DEADLOCK_SQL_CODE)
+ || // Deadlock detected
+ sqlState.equals(SERIALIZATION_FAILURE_SQL_CODE); // Serialization
failure
+ }
+
+ // Additionally, one might check for specific error messages or other
conditions
+ return e.getMessage().contains("connection refused")
+ || e.getMessage().contains("connection reset");
+ }
+
+ public <T> T withRetries(Operation<T> operation) throws SQLException {
+ int attempts = 0;
+ // maximum number of retries.
+ int maxAttempts = relationalJdbcConfiguration.maxRetries().orElse(1);
+ // How long we should try, since the first attempt.
+ long maxDuration =
relationalJdbcConfiguration.maxDurationInMs().orElse(100L);
+ // How long to wait before first failure.
+ long delay = relationalJdbcConfiguration.initialDelayInMs().orElse(100L);
+
+ // maximum time we will retry till.
+ long maxRetryTime = Instant.now().toEpochMilli() + maxDuration;
+
+ while (attempts < maxAttempts) {
try {
- try (Statement statement = connection.createStatement()) {
- success = callback.execute(statement);
+ return operation.execute();
+ } catch (SQLException e) {
+ attempts++;
+ long timeLeft = Math.max((maxRetryTime -
Instant.now().toEpochMilli()), 0L);
Review Comment:
Please consider using a `Clock` instead of `Instant.now()`... it can be very
useful in tests (now or later). Default can be `Clock.systemUTC()`.
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.relational.jdbc;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Optional;
+
+@ConfigMapping(prefix = "polaris.persistence.relational.jdbc")
Review Comment:
I believe this should be kept-inside quarkus-specific module... That is, the
JDBC module can declare a config interface, but quarkus module will have to
extend it and provide config annotations.
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -173,23 +190,82 @@ public int executeUpdate(String query) throws
SQLException {
* @throws SQLException : Exception caught during transaction execution.
*/
public void runWithinTransaction(TransactionCallback callback) throws
SQLException {
- try (Connection connection = borrowConnection()) {
- boolean autoCommit = connection.getAutoCommit();
- connection.setAutoCommit(false);
- boolean success = false;
+ withRetries(
+ () -> {
+ try (Connection connection = borrowConnection()) {
+ boolean autoCommit = connection.getAutoCommit();
+ boolean success = false;
+ connection.setAutoCommit(false);
+ try {
+ try (Statement statement = connection.createStatement()) {
+ success = callback.execute(statement);
+ }
+ } finally {
+ if (success) {
+ connection.commit();
+ } else {
+ connection.rollback();
+ }
+ connection.setAutoCommit(autoCommit);
+ }
+ }
+ return null;
+ });
+ }
+
+ private boolean isRetryable(SQLException e) {
+ String sqlState = e.getSQLState();
+
+ if (sqlState != null) {
+ return sqlState.equals(DEADLOCK_SQL_CODE)
+ || // Deadlock detected
+ sqlState.equals(SERIALIZATION_FAILURE_SQL_CODE); // Serialization
failure
+ }
+
+ // Additionally, one might check for specific error messages or other
conditions
+ return e.getMessage().contains("connection refused")
+ || e.getMessage().contains("connection reset");
+ }
+
+ public <T> T withRetries(Operation<T> operation) throws SQLException {
+ int attempts = 0;
+ // maximum number of retries.
+ int maxAttempts = relationalJdbcConfiguration.maxRetries().orElse(1);
Review Comment:
IMHO, max attempts should not be restricted by default (MAX_VALUE). Only
time should be restricted.
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -173,23 +190,82 @@ public int executeUpdate(String query) throws
SQLException {
* @throws SQLException : Exception caught during transaction execution.
*/
public void runWithinTransaction(TransactionCallback callback) throws
SQLException {
- try (Connection connection = borrowConnection()) {
- boolean autoCommit = connection.getAutoCommit();
- connection.setAutoCommit(false);
- boolean success = false;
+ withRetries(
+ () -> {
+ try (Connection connection = borrowConnection()) {
+ boolean autoCommit = connection.getAutoCommit();
+ boolean success = false;
+ connection.setAutoCommit(false);
+ try {
+ try (Statement statement = connection.createStatement()) {
+ success = callback.execute(statement);
+ }
+ } finally {
+ if (success) {
+ connection.commit();
+ } else {
+ connection.rollback();
+ }
+ connection.setAutoCommit(autoCommit);
+ }
+ }
+ return null;
+ });
+ }
+
+ private boolean isRetryable(SQLException e) {
+ String sqlState = e.getSQLState();
+
+ if (sqlState != null) {
+ return sqlState.equals(DEADLOCK_SQL_CODE)
+ || // Deadlock detected
+ sqlState.equals(SERIALIZATION_FAILURE_SQL_CODE); // Serialization
failure
+ }
+
+ // Additionally, one might check for specific error messages or other
conditions
+ return e.getMessage().contains("connection refused")
+ || e.getMessage().contains("connection reset");
+ }
+
+ public <T> T withRetries(Operation<T> operation) throws SQLException {
+ int attempts = 0;
+ // maximum number of retries.
+ int maxAttempts = relationalJdbcConfiguration.maxRetries().orElse(1);
+ // How long we should try, since the first attempt.
+ long maxDuration =
relationalJdbcConfiguration.maxDurationInMs().orElse(100L);
+ // How long to wait before first failure.
+ long delay = relationalJdbcConfiguration.initialDelayInMs().orElse(100L);
+
+ // maximum time we will retry till.
+ long maxRetryTime = Instant.now().toEpochMilli() + maxDuration;
+
+ while (attempts < maxAttempts) {
try {
- try (Statement statement = connection.createStatement()) {
- success = callback.execute(statement);
+ return operation.execute();
+ } catch (SQLException e) {
+ attempts++;
+ long timeLeft = Math.max((maxRetryTime -
Instant.now().toEpochMilli()), 0L);
+ if (attempts >= maxAttempts || !isRetryable(e) || timeLeft == 0) {
+ throw e;
}
- } finally {
- if (success) {
- connection.commit();
- } else {
- connection.rollback();
+ // Add jitter
+ long timeToSleep = Math.min(timeLeft, delay + (long)
(random.nextDouble() * 0.2 * delay));
Review Comment:
So this formula give jitter in the range `[0, 0.2 * delay]`. This is too
narrow IMHO. The purpose of jitter is to minimize the chance of secondary
collisions. I'd use the jitter range of `(0, delay]`. Please be careful with
`0` sleep time.
I personally do not see value in having a fixed component (`delay`) in the
sleep time. Merely delaying an operation is not helpful and we have not way of
predicting when remote transactions are going to finish.
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -173,23 +190,82 @@ public int executeUpdate(String query) throws
SQLException {
* @throws SQLException : Exception caught during transaction execution.
*/
public void runWithinTransaction(TransactionCallback callback) throws
SQLException {
- try (Connection connection = borrowConnection()) {
- boolean autoCommit = connection.getAutoCommit();
- connection.setAutoCommit(false);
- boolean success = false;
+ withRetries(
+ () -> {
+ try (Connection connection = borrowConnection()) {
+ boolean autoCommit = connection.getAutoCommit();
+ boolean success = false;
+ connection.setAutoCommit(false);
+ try {
+ try (Statement statement = connection.createStatement()) {
+ success = callback.execute(statement);
+ }
+ } finally {
+ if (success) {
+ connection.commit();
+ } else {
+ connection.rollback();
+ }
+ connection.setAutoCommit(autoCommit);
+ }
+ }
+ return null;
+ });
+ }
+
+ private boolean isRetryable(SQLException e) {
+ String sqlState = e.getSQLState();
+
+ if (sqlState != null) {
+ return sqlState.equals(DEADLOCK_SQL_CODE)
+ || // Deadlock detected
+ sqlState.equals(SERIALIZATION_FAILURE_SQL_CODE); // Serialization
failure
+ }
+
+ // Additionally, one might check for specific error messages or other
conditions
+ return e.getMessage().contains("connection refused")
+ || e.getMessage().contains("connection reset");
+ }
+
+ public <T> T withRetries(Operation<T> operation) throws SQLException {
+ int attempts = 0;
+ // maximum number of retries.
+ int maxAttempts = relationalJdbcConfiguration.maxRetries().orElse(1);
+ // How long we should try, since the first attempt.
+ long maxDuration =
relationalJdbcConfiguration.maxDurationInMs().orElse(100L);
+ // How long to wait before first failure.
+ long delay = relationalJdbcConfiguration.initialDelayInMs().orElse(100L);
+
+ // maximum time we will retry till.
+ long maxRetryTime = Instant.now().toEpochMilli() + maxDuration;
+
+ while (attempts < maxAttempts) {
try {
- try (Statement statement = connection.createStatement()) {
- success = callback.execute(statement);
+ return operation.execute();
+ } catch (SQLException e) {
+ attempts++;
+ long timeLeft = Math.max((maxRetryTime -
Instant.now().toEpochMilli()), 0L);
+ if (attempts >= maxAttempts || !isRetryable(e) || timeLeft == 0) {
+ throw e;
Review Comment:
Please add retries info (e.g. add a suppressed exception with retry info in
the message).
Also, it could be valuable to add previous exceptions as "suppressed".
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
