svn commit: r1604636 - in /logging/log4j/log4j2/trunk/log4j-nosql/src: main/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManager.java test/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManagerTest.java

[ma...@apache.org]

Author: mattsicker
Date: Sun Jun 22 18:33:14 2014
New Revision: 1604636

URL: http://svn.apache.org/r1604636
Log:
Don't close NoSQL clients on commit.

  - Related to LOG4J2-676.
  - Our NoSQL drivers are actually clients which means calling close() on them (or equivalent) is really more like shutting down the connection pool than closing an individual connection.

Modified:
    logging/log4j/log4j2/trunk/log4j-nosql/src/main/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManager.java
    logging/log4j/log4j2/trunk/log4j-nosql/src/test/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManagerTest.java

Modified: logging/log4j/log4j2/trunk/log4j-nosql/src/main/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManager.java
URL: http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-nosql/src/main/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManager.java?rev=1604636&r1=1604635&r2=1604636&view=diff
==============================================================================
--- logging/log4j/log4j2/trunk/log4j-nosql/src/main/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManager.java (original)
+++ logging/log4j/log4j2/trunk/log4j-nosql/src/main/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManager.java Sun Jun 22 18:33:14 2014
@@ -51,6 +51,7 @@ public final class NoSQLDatabaseManager<
 
     @Override
     protected void shutdownInternal() {
+        // NoSQL doesn't use transactions, so all we need to do here is simply close the client
         Closer.closeSilent(this.connection);
     }
 
@@ -155,13 +156,10 @@ public final class NoSQLDatabaseManager<
 
     @Override
     protected void commitAndClose() {
-        try {
-            if (this.connection != null && !this.connection.isClosed()) {
-                this.connection.close();
-            }
-        } catch (Exception e) {
-            throw new AppenderLoggingException("Failed to commit and close NoSQL connection in manager.", e);
-        }
+        // all NoSQL drivers auto-commit (since NoSQL doesn't generally use the concept of transactions).
+        // also, all our NoSQL drivers use internal connection pooling and provide clients, not connections.
+        // thus, we should not be closing the client until shutdown as NoSQL is very different from SQL.
+        // see LOG4J2-591 and LOG4J2-676
     }
 
     private NoSQLObject<W>[] convertStackTrace(final StackTraceElement[] stackTrace) {

Modified: logging/log4j/log4j2/trunk/log4j-nosql/src/test/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManagerTest.java
URL: http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-nosql/src/test/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManagerTest.java?rev=1604636&r1=1604635&r2=1604636&view=diff
==============================================================================
--- logging/log4j/log4j2/trunk/log4j-nosql/src/test/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManagerTest.java (original)
+++ logging/log4j/log4j2/trunk/log4j-nosql/src/test/java/org/apache/logging/log4j/nosql/appender/NoSQLDatabaseManagerTest.java Sun Jun 22 18:33:14 2014
@@ -71,14 +71,6 @@ public class NoSQLDatabaseManagerTest {
             replay(this.provider, this.connection);
 
             manager.connectAndStart();
-
-            verify(this.provider, this.connection);
-            reset(this.provider, this.connection);
-            expect(this.connection.isClosed()).andReturn(false);
-            this.connection.close();
-            expectLastCall();
-            replay(this.provider, this.connection);
-
             manager.commitAndClose();
         } finally {
             try {
@@ -148,13 +140,6 @@ public class NoSQLDatabaseManagerTest {
             } catch (AppenderLoggingException ignore) {
                 /* */
             }
-
-            verify(this.provider, this.connection, event);
-            reset(this.provider, this.connection);
-            expect(this.connection.isClosed()).andReturn(false);
-            this.connection.close();
-            expectLastCall();
-            replay(this.provider, this.connection);
         } finally {
             try {
                 manager.release();
@@ -242,11 +227,6 @@ public class NoSQLDatabaseManagerTest {
             assertNull("The context stack should be null.", object.get("contextStack"));
 
             verify(this.provider, this.connection, event, message);
-            reset(this.provider, this.connection);
-            expect(this.connection.isClosed()).andReturn(false);
-            this.connection.close();
-            expectLastCall();
-            replay(this.provider, this.connection);
         } finally {
             try {
                 manager.release();
@@ -382,11 +362,6 @@ public class NoSQLDatabaseManagerTest {
             assertEquals("The context stack is not correct.", stack.asList(), object.get("contextStack"));
 
             verify(this.provider, this.connection, event, message);
-            reset(this.provider, this.connection);
-            expect(this.connection.isClosed()).andReturn(false);
-            this.connection.close();
-            expectLastCall();
-            replay(this.provider, this.connection);
         } finally {
             try {
                 manager.release();
@@ -584,11 +559,6 @@ public class NoSQLDatabaseManagerTest {
             assertEquals("The context stack is not correct.", stack.asList(), object.get("contextStack"));
 
             verify(this.provider, this.connection, event, message);
-            reset(this.provider, this.connection);
-            expect(this.connection.isClosed()).andReturn(false);
-            this.connection.close();
-            expectLastCall();
-            replay(this.provider, this.connection);
         } finally {
             try {
                 manager.release();