Decouple session from connection #324

lutovich · 2017-02-23T10:32:56Z

No description provided.

Previously there existed two driver implementations `DirectDriver` and `RoutingDriver`. Main difference was that first one used connection pool directly while the second one used load balancing connection pool. This commit introduces an interface to unify connection pool and load balancer and collapses two driver implementations into one that uses this interface. This also makes it more convenient to decouple connection from session because session can now use this same single interface without knowing if it is a direct or routing session.

So it can use the provider to acquire connections when needed. This is a piece of preparatory work to decouple session from connection. This commit does not change how sessions use connections.

Previously session was bound to a connection and used this same connection throughout its lifetime. This means that there was no load-balancing within single session and that connection could have been claimed by the session for a long time without actually being used. This commit makes session obtain new connections when needed and return them to the pool as soon as possible. New connection is obtained for: * each `Session#run()` * each `Session#beginTransaction()` and created transaction holds same connection throughout its lifetime Connection is returned back to the pool when either: * result is fully read into memory * transaction is closed * subsequent `Session#run()` is invoked Introduced new interface `ConnectionHandler` to manage connections around session, result and transaction. `NetworkSession` is the only implementation of this interface. Also added new `DriverClosedException` thrown on every interaction with a closed driver.

Hash codes instead of UUID strings for session/connection identification, removed `Connection#logger()` interface method.

zhenlineo · 2017-02-23T10:44:40Z

driver/src/main/java/org/neo4j/driver/internal/InternalDriver.java

        {
            // the driver is already closed and we either 1. obtain this session from the old session pool
            // or 2. we obtain this session from a new session pool
            // For 1. this closeResources will take no effect as everything is already closed.
            // For 2. this closeResources will close the new connection pool just created to ensure no resource leak.
            closeResources();
-            throw driverCloseException();
+            throw new DriverClosedException();


Maybe not a new exception type in driver for now

zhenlineo · 2017-02-23T10:47:28Z

driver/src/main/java/org/neo4j/driver/internal/InternalStatementResult.java

@@ -319,6 +321,7 @@ private boolean tryFetchNext()
        {
            if ( done )
            {
+                connectionHandler.resultBuffered();


if you do connectionHandler.resultBuffered() at the place where done is set to be true, then you probably could also address failure/ignore cases too.

zhenlineo · 2017-02-23T10:53:48Z

driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java

-        connection.resetAsync();
+        if ( currentConnection != null )
+        {
+            currentConnection.resetAsync();


I would suggest to remove reset. Make this method deprecated and then give an empty impl inside.

Can we maybe remove it in a separate PR? This one is quite big already :)

zhenlineo · 2017-02-23T11:12:10Z

driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java

+        if ( currentTransaction != null && currentTransaction == tx )
+        {
+            lastBookmark = currentTransaction.bookmark();
+            currentTransaction = null;


connection also need to be closed

zhenlineo · 2017-02-23T11:18:34Z

driver/src/main/java/org/neo4j/driver/internal/net/pooling/PooledSocketConnection.java

@@ -58,7 +59,7 @@
    private final Consumer<PooledConnection> release;

    private boolean unrecoverableErrorsOccurred = false;
-    private Runnable onError = null;
+    private ConnectionHandler connectionHandler;


suggesting rename something else such as ConnectionTerminationHandler?

To check that it handles connections properly.

And improved method names to better reflect the purpose of the interface.

All usages changed back to IllegalStateException. This revert is done because we should first refine driver errors hierarchy and only then introduce specific exception types.

When result fetching fails (cypher runtime error or network error) exception is thrown but it is still possible to maintain a reference to the result object. It'll just be empty after failure. It is not possible to fetch any other records after failure and the underlying pooled connection can be closed and returned to the pool. This commit makes result return its connection to the pool on failure. This connection (if healthy) can be used by subsequent sessions. Also added tests for connection handling.

zhenlineo · 2017-02-24T09:39:05Z

driver/src/test/java/org/neo4j/driver/internal/InternalStatementResultTest.java

+        SessionResourcesHandler resourcesHandler = mock( SessionResourcesHandler.class );
+        StatementResult result = createResult( 10, resourcesHandler );
+
+        assertNotNull( result.summary() );


maybe summary several times to ensure only called once?
Similar to other methods and also some combination of all the methods? such as result.single(), result.summary(), then result.consume()?

Added a separate test for this here: 0ddac3d

Added a check to sync only open connections before closing them in the session.

lutovich added 5 commits February 21, 2017 16:49

Makes NetworkSession aware of ConnectionProvider

2bab0a7

So it can use the provider to acquire connections when needed. This is a piece of preparatory work to decouple session from connection. This commit does not change how sessions use connections.

Remove unused field from InternalStatementResult

c87a0db

Simplify logging around sessions and connections

68019a1

Hash codes instead of UUID strings for session/connection identification, removed `Connection#logger()` interface method.

lutovich added the NOT READY FOR MERGE label Feb 23, 2017

zhenlineo reviewed Feb 23, 2017

View reviewed changes

lutovich added 4 commits February 23, 2017 13:50

More unit tests for NetworkSession

99b17c3

To check that it handles connections properly.

Renamed ConnectionHandler to SessionResourcesHandler

da96356

And improved method names to better reflect the purpose of the interface.

Removed DriverClosedException

23e570a

All usages changed back to IllegalStateException. This revert is done because we should first refine driver errors hierarchy and only then introduce specific exception types.

lutovich removed the NOT READY FOR MERGE label Feb 23, 2017

lutovich changed the title ~~Separate conn from session~~ Decouple session from connection Feb 23, 2017

zhenlineo reviewed Feb 24, 2017

View reviewed changes

lutovich added 2 commits February 24, 2017 11:32

Test for fetching result multiple times

0ddac3d

Sync only open connections

d860477

Added a check to sync only open connections before closing them in the session.

zhenlineo merged commit 378ae43 into neo4j:1.2 Feb 24, 2017

lutovich deleted the 1.2-separate-conn-from-session branch February 24, 2017 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple session from connection #324

Decouple session from connection #324

lutovich commented Feb 23, 2017

zhenlineo Feb 23, 2017

zhenlineo Feb 23, 2017

zhenlineo Feb 23, 2017

lutovich Feb 23, 2017

zhenlineo Feb 23, 2017

zhenlineo Feb 23, 2017

zhenlineo Feb 24, 2017

lutovich Feb 24, 2017

Decouple session from connection #324

Decouple session from connection #324

Conversation

lutovich commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment