jackrabbit-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From marti...@apache.org
Subject 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
Date Thu, 12 Nov 2009 12:18:43 GMT
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;
             }



Mime
View raw message