Skip to content

Initial support for async in routing driver #409

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 4, 2017

Conversation

lutovich
Copy link
Contributor

No description provided.

Copy link
Contributor

@ali-ince ali-ince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I added a couple of comments. Besides, having both ConnectionPool and AsyncConnectionPool in the code base started to be confusing for me, maybe it is time for a clean-up if those old pieces are not needed anymore.

@@ -33,11 +33,15 @@
*/
BoltServerAddress selectReader( BoltServerAddress[] knownReaders );

BoltServerAddress selectReaderAsync( BoltServerAddress[] knownReaders );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

@@ -193,6 +286,21 @@ private BoltServerAddress selectAddress( AccessMode mode, AddressSet servers )
}
}

private BoltServerAddress selectAddressAsync( AccessMode mode, AddressSet servers )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this sync/async distinction at this level? Do you have plans to implement selection differently in the future?

@@ -47,11 +47,23 @@ public BoltServerAddress selectReader( BoltServerAddress[] knownReaders )
}

@Override
public BoltServerAddress selectReaderAsync( BoltServerAddress[] knownReaders )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

this.routingTable = routingTable;
this.rediscovery = rediscovery;
this.loadBalancingStrategy = loadBalancingStrategy;
this.log = log;

refreshRoutingTable();
if ( connections != null )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this depend only on connections? Or should asyncConnectionPool has a say in the decision?

@lutovich
Copy link
Contributor Author

lutovich commented Oct 2, 2017

@ali-ince thanks for the review! I planned to get rid of those *Async suffixes and pool duality in the next step when doing blocking on top of async.

To return both result and invoked procedure at the same time.
Previously procedure had to be accessed separately via getter. This is
a preparation for async, where full result will make it easier to
chain futures.
This commit makes `RoutingProcedureRunner` able to invoke routing
procedures asynchronously using given connection future. Server
version check is performed to determine which procedure to call.
This commit makes `ClusterCompositionProvider` able to interpret async
responses from `RoutingProcedureRunner`.
This commit makes `LoadBalancer` able to serve async connections.
Rediscovery procedure is also executed asynchronously. Connections
returned by `LoadBalancer` have special error handling logic to
remove failed instances from the routing table and purge their
connections from the pool.

Also made `AsyncConnection` expose server address and version
instead of `ServerInfo`.

Couple TODOs are introduced. They will be addressed in the
subsequent commits.
@zhenlineo zhenlineo merged commit c6ae295 into neo4j:1.5 Oct 4, 2017
@lutovich lutovich deleted the 1.5-async-routing branch October 4, 2017 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants