-
Notifications
You must be signed in to change notification settings - Fork 155
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
Conversation
2f633c2
to
d205c75
Compare
There was a problem hiding this 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 ); |
There was a problem hiding this comment.
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 ) |
There was a problem hiding this comment.
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 ) |
There was a problem hiding this comment.
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 ) |
There was a problem hiding this comment.
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?
@ali-ince thanks for the review! I planned to get rid of those |
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.
d205c75
to
c6ae295
Compare
No description provided.