Author: martijnh Date: Thu Nov 12 12:18:43 2009 New Revision: 835363 URL: http://svn.apache.org/viewvc?rev=835363&view=rev Log: JCR-1456 Database connection pooling Improved handling of connection failures Modified: jackrabbit/sandbox/JCR-1456/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/BundleDbPersistenceManager.java jackrabbit/sandbox/JCR-1456/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/ConnectionHelper.java Modified: jackrabbit/sandbox/JCR-1456/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/BundleDbPersistenceManager.java URL: http://svn.apache.org/viewvc/jackrabbit/sandbox/JCR-1456/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/BundleDbPersistenceManager.java?rev=835363&r1=835362&r2=835363&view=diff ============================================================================== --- jackrabbit/sandbox/JCR-1456/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/BundleDbPersistenceManager.java (original) +++ jackrabbit/sandbox/JCR-1456/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/BundleDbPersistenceManager.java Thu Nov 12 12:18:43 2009 @@ -466,11 +466,14 @@ * {@inheritDoc} * * Basically wraps a JDBC transaction around super.store(). + * + * FIXME: the retry logic is almost a duplicate of {@code ConnectionHelper.RetryManager}. */ - public synchronized void store(ChangeLog changeLog) throws ItemStateException { + public synchronized void store(final ChangeLog changeLog) throws ItemStateException { int failures = 0; ItemStateException lastException = null; - while (failures <= 1) { // retry once + boolean sleepInterrupted = false; + while (!sleepInterrupted && (blockOnConnectionLoss || failures <= 1)) { try { conHelper.startBatch(); super.store(changeLog); @@ -479,11 +482,9 @@ } catch (SQLException e) { // Either startBatch or stopBatch threw it: either way the // transaction was not persisted and no action needs to be taken. - failures++; lastException = new ItemStateException(e.getMessage(), e); } catch (ItemStateException e) { // store call threw it: we need to cancel the transaction - failures++; lastException = e; try { conHelper.endBatch(false); @@ -491,6 +492,19 @@ DbUtility.logException("rollback failed", e2); } } + failures++; + log.error("Failed to persist ChangeLog (stacktrace on DEBUG log level), blockOnConnectionLoss = " + + blockOnConnectionLoss, lastException); + log.debug("Failed to persist ChangeLog", lastException); + if (blockOnConnectionLoss || failures <= 1) { // if we're going to try again + try { + Thread.sleep(100); + } catch (InterruptedException e1) { + Thread.currentThread().interrupt(); + sleepInterrupted = true; + log.error("Interrupted: canceling retry of ChangeLog storage"); + } + } } throw lastException; } Modified: jackrabbit/sandbox/JCR-1456/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/ConnectionHelper.java URL: http://svn.apache.org/viewvc/jackrabbit/sandbox/JCR-1456/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/ConnectionHelper.java?rev=835363&r1=835362&r2=835363&view=diff ============================================================================== --- jackrabbit/sandbox/JCR-1456/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/ConnectionHelper.java (original) +++ jackrabbit/sandbox/JCR-1456/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/ConnectionHelper.java Thu Nov 12 12:18:43 2009 @@ -25,15 +25,51 @@ import javax.sql.DataSource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + /** * This class provides convenience methods to execute SQL statements. They can be either executed in isolation - * or with in the context of a JDBC transaction (use the {@link #startBatch()} and {@link #endBatch(boolean)} - * methods for this). + * or within the context of a JDBC transaction; the so-called batch mode (use the {@link #startBatch()} + * and {@link #endBatch(boolean)} methods for this). + * + *

+ * + * This class contains logic to retry execution of SQL statements. If this helper is not in batch mode + * and if a statement fails due to an {@code SQLException}, then it is retried. If the {@code block} argument + * of the constructor call was {@code false} then it is retried only once. Otherwise the statement is retried + * until either it succeeds or the thread is interrupted. This clearly assumes that the only cause of {@code + * SQLExceptions} is faulty {@code Connections} which are restored eventually.
Note: + * This retry logic only applies to the following methods: + *

+ * + *

+ * + * This class is not thread-safe and if it is to be used by multiple threads then the clients must make sure + * that access to this class is properly synchronized. + * + *

+ * + * Implementation note: The {@code Connection} that is retrieved from the {@code DataSource} + * in {@link #getConnection()} may be broken. This is so because if an internal {@code DataSource} is used, + * then this is a commons-dbcp {@code DataSource} with a testWhileIdle validation strategy (see + * the {@code ConnectionFactory} class). Furthermore, if it is a {@code DataSource} obtained through JNDI then we + * can make no assumptions about the validation strategy. This means that our retry logic must either assume that + * the SQL it tries to execute can do so without errors (i.e., the statement is valid), or it must implement its + * own validation strategy to apply. Currently, the former is in place. */ public class ConnectionHelper { + private static Logger log = LoggerFactory.getLogger(ConnectionHelper.class); + private static final int RETRIES = 1; + private static final int SLEEP_BETWEEN_RETRIES_MS = 100; + private final boolean blockOnConnectionLoss; private final boolean checkTablesWithUserName; @@ -46,7 +82,7 @@ /** * @param dataSrc the {@link DataSource} on which this instance acts - * @param block whether the helper should transparantly block on DB connection loss (otherwise it retries + * @param block whether the helper should transparently block on DB connection loss (otherwise it retries * once and if that fails throws exception) */ public ConnectionHelper(DataSource dataSrc, boolean block) { @@ -58,7 +94,7 @@ /** * @param dataSrc the {@link DataSource} on which this instance acts * @param checkWithUserName whether the username is to be used for the {@link #tableExists(String)} method - * @param block whether the helper should transparantly block on DB connection loss (otherwise it throws exceptions) + * @param block whether the helper should transparently block on DB connection loss (otherwise it throws exceptions) */ protected ConnectionHelper(DataSource dataSrc, boolean checkWithUserName, boolean block) { dataSource = dataSrc; @@ -70,6 +106,8 @@ * A utility method that makes sure that identifier does only consist of characters that are * allowed in names on the target database. Illegal characters will be escaped as necessary. * + * This method is not affected by the + * * @param identifier the identifier to convert to a db specific identifier * @return the db-normalized form of the given identifier * @throws SQLException if an error occurs @@ -156,9 +194,9 @@ } /** - * Starts the batch mode. If an {@link SQLException} is thrown, then the batch mode is not started.

- * Important: clients that call this method must make sure that {@link #endBatch(boolean)} is called - * eventually. + * Starts the batch mode. If an {@link SQLException} is thrown, then the batch mode is not started.

+ * Important: clients that call this method must make sure that + * {@link #endBatch(boolean)} is called eventually. * * @throws SQLException on error */ @@ -181,10 +219,11 @@ } /** - * This method always ends the batch mode. + * This method always ends the batch mode. * * @param commit whether the changes in the batch should be committed or rolled back - * @throws SQLException on error + * @throws SQLException if the commit or rollback of the underlying JDBC Connection threw an {@code + * SQLException} */ public final void endBatch(boolean commit) throws SQLException { if (!inBatchMode) { @@ -342,7 +381,7 @@ if (inBatchMode) { return batchConnection; } else { - Connection con = getConnectionFromDS(); + Connection con = dataSource.getConnection(); // JCR-1013: Setter may fail unnecessarily on a managed connection if (!con.getAutoCommit()) { con.setAutoCommit(true); @@ -352,35 +391,6 @@ } /** - * This method retries if {@code dataSource.getConnection()} throws an SQLException and sleeping - * is not interrupted. This can happen when the database server is down. - * - * @return a {@code Connection} - * @throws SQLException on error - */ - private Connection getConnectionFromDS() throws SQLException { - Connection con = null; - SQLException lastException = null; - boolean sleepInterrupted = false; - int failures = 0; - while (con == null && !sleepInterrupted && (blockOnConnectionLoss || failures <= RETRIES)) { - try { - return dataSource.getConnection(); - } catch (SQLException e) { - failures++; - lastException = e; - try { - Thread.sleep(100); - } catch (InterruptedException e1) { - Thread.currentThread().interrupt(); - sleepInterrupted = true; - } - } - } - throw lastException; // guaranteed to be non-null - } - - /** * Closes the given resources given the {@code batchMode} state. * * @param con the {@code Connection} obtained through the {@link #getConnection()} method @@ -409,6 +419,7 @@ protected PreparedStatement execute(PreparedStatement stmt, Object[] params) throws SQLException { for (int i = 0; params != null && i < params.length; i++) { Object p = params[i]; + // FIXME: what about already consumed input streams when in a retry? if (p instanceof StreamWrapper) { StreamWrapper wrapper = (StreamWrapper) p; stmt.setBinaryStream(i + 1, wrapper.getStream(), (int) wrapper.getSize()); @@ -431,15 +442,27 @@ if (inBatchMode) { return call(); } else { + boolean sleepInterrupted = false; int failures = 0; SQLException lastException = null; - while (failures <= RETRIES) { + while (!sleepInterrupted && (blockOnConnectionLoss || failures <= RETRIES)) { try { return call(); } catch (SQLException e) { - failures++; lastException = e; } + log.error("Failed to execute SQL (stacktrace on DEBUG log level)", lastException); + log.debug("Failed to execute SQL", lastException); + failures++; + if (blockOnConnectionLoss || failures <= RETRIES) { // if we're going to try again + try { + Thread.sleep(SLEEP_BETWEEN_RETRIES_MS); + } catch (InterruptedException e1) { + Thread.currentThread().interrupt(); + sleepInterrupted = true; + log.error("Interrupted: canceling retry"); + } + } } throw lastException; }