Mailing-List: contact dev-help@jackrabbit.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@jackrabbit.apache.org
Message-ID: <7416579.1189154910875.JavaMail.jira@brutus>
Date: Fri, 7 Sep 2007 01:48:30 -0700 (PDT)
From: "Thomas Mueller (JIRA)" <jira@apache.org>
To: dev@jackrabbit.apache.org
Subject: [jira] Commented: (JCR-940) add db connection autoConnect for
 BundleDbPersistenceManager.
In-Reply-To: <14540291.1180021276911.JavaMail.jira@brutus>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/JCR-940?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12525653 ] 

Thomas Mueller commented on JCR-940:
------------------------------------

> store: log.warn("storing and committing changes failed: " + e.getMessage());

exception stack traces must be logged as well:

log.warn("storing and committing changes failed: " + e.getMessage(), e);

> stmt = connectionManager.getConnection().prepareStatement("select NODE_ID, BUNDLE_DATA from BUNDLE");

The SQL statement should stored in one of the member variables in my view (but it was also not done before...).

+        if (!connectionManager.setAutoCommit(false)) {
+            throw new ItemStateException("disabling autoCommit failed");

Using a return value for an exception.

+            Object[] keys = getKey(bundle.getId().getUUID());
+            Object[] params = new Object[keys.length + 1];
+            params[0] = out.toByteArray();
+            for (int i = 1; i < params.length; i++) {
+                params[i] = keys[i-1];
             }

What about using an ArrayList:
List params = getKey(bundle.getId().getUUID());
params.add(out.toByteArray());

-            stmt.setBytes(2, bundle.getId().getUUID().getRawBytes());
This is code you removed... Strange, how did this work with getStorageModel() != SM_BINARY_KEYS?

+            Connection con = connectionManager.getConnection();
+            if (con != null) {
+                DatabaseMetaData metaData = con.getMetaData();
+                if (metaData.getDriverMajorVersion() < 10) {

+            Connection con = connectionManager.getConnection();
+            if (con != null) {
+                try {
+                    con.rollback();

In some places the return value is checked, in other places not. I would make sure getConnection never returns null, otherwise you get NullPointerExceptions in strange places, and you don't really know what was the reason.

+    /**
+     * @return the database connection that is managed, possibly null
+     */
+    public synchronized Connection getConnection() {
+        int trials = TRIALS;
+        while (trials-- > 0) {
+
+            // First, try to reconnect if needed
+            if (isClosed && autoReconnect) {
+                reestablishConnection();
+            }
+
+            // Then, try to return the connection
+            if (!isClosed) {
+                return connection;
+            }
+        }
+        log.warn("failed to get connection");
+        return null;
+    }

I would write:

+    /**
+     * @return the database connection that is managed, possibly null
+     */
+    public synchronized Connection getConnection() throws SQLException {
+        // reconnect if needed
+        if (isClosed && autoReconnect) {
+            reestablishConnection();
+        }
+        return connection;
+    }

In my view, reestablishConnection should loop, and if it can't connect, throw an exception. And once it failed, it should set autoReconnect to false, otherwise other code will try to re-connect again and again (maybe endlessly). You have used reestablishConnection in many places in the code, I think it should only be called in one place.

Thomas


> add db connection autoConnect for BundleDbPersistenceManager.
> -------------------------------------------------------------
>
>                 Key: JCR-940
>                 URL: https://issues.apache.org/jira/browse/JCR-940
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: core
>    Affects Versions: 1.3
>            Reporter: Xiaohua Lu
>         Attachments: JCR-940-v2.patch, JCR-940.patch
>
>
> Since bundled db pm doesn't inherited from database pm, it can't reconnect once database is bounced. it would be nice to add this feature. 

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.