You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@jackrabbit.apache.org by ma...@apache.org on 2009/11/12 13:18:43 UTC
svn commit: r835363 - in
/jackrabbit/sandbox/JCR-1456/jackrabbit-core/src/main/java/org/apache/jackrabbit/core:
persistence/bundle/BundleDbPersistenceManager.java
util/db/ConnectionHelper.java
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 <i>batch mode</i> (use the {@link #startBatch()}
+ * and {@link #endBatch(boolean)} methods for this).
+ *
+ * <p/>
+ *
+ * This class contains logic to retry execution of SQL statements. If this helper is <i>not</i> 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. <br/> <strong>Note</strong>:
+ * This retry logic only applies to the following methods:
+ * <ul>
+ * <li>{@link #exec(String, Object...)}</li>
+ * <li>{@link #update(String, Object[])}</li>
+ * <li>{@link #exec(String, Object[], boolean, int)}</li>
+ * </ul>
+ *
+ * <p/>
+ *
+ * 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.
+ *
+ * <p/>
+ *
+ * <strong>Implementation note</strong>: 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 <code>testWhileIdle</code> 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 <code>identifier</code> 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. <p/>
- * Important: clients that call this method must make sure that {@link #endBatch(boolean)} is called
- * eventually.
+ * Starts the <i>batch mode</i>. If an {@link SQLException} is thrown, then the batch mode is not started. <p/>
+ * <strong>Important:</strong> 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 <i>batch mode</i>.
*
* @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;
}