Skip to content
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

Routing driver #115

Merged
merged 11 commits into from
Nov 11, 2016
Merged

Routing driver #115

merged 11 commits into from
Nov 11, 2016

Conversation

zhenlineo
Copy link
Contributor

@zhenlineo zhenlineo commented Oct 7, 2016

Created ClusterConnectionPool as pool of existing connection pools.

Added new scheme bolt+routing for creating a new RoutingDriver for routing queries to a cluster.

  • The RoutingDriver holds a Loadbalancer which is responsible for supplying an usable connection during the creation of a specific read/write session.
  • The Loadbalancer holds a ClusterView which describes the client's current understanding of the servers in the cluster and a ClusterConnectionPool that maintains a dictionary of {server, connection pool}.
  • The servers in ClusterView might become unavailable and get (temporarily or permanently) removed. The ClusterConnectionPool could choose to either still keep the connection pool of a unavailable server or remove it totally too. However it is illegal to have servers that are known to ClusterView but have no connection pool entry in ClusterConnectionPool.
  • The ClusterView load balance the connections to the servers using a ConcurrentRoundRobinSet.

Added error handling callback in SocketConnection.

  • Moved the logic of verifying if a pooled connection is re-usable from SocketConnection to PooledConnection
  • The purge of the servers and connection pools in Loadbalancer is done via a error handling event callback in SocketConnection passed by LoadBalancer.

TODO:
Feature: Retry rediscovery?
ITs

Zhen Li added 6 commits October 7, 2016 17:43
Created ClusterConnectionPool as pool of existing connection pools
Added RoutingDriver for `bolt+routing` scheme.
Added the basic logic of `CheckServers`

TODO: Discovery, multithread support, error handling
Added a load balancer who has the full control of a cluster view and a cluster connection pool
Added discovery manager who takes a connection and perform discovery to save the new cluster status for creating a new cluster view

What is missing now:
timeout, error handling, remove connection when session/tx throw connection errors
@zhenlineo zhenlineo force-pushed the 1.1-discovery-and-routing branch 3 times, most recently from a988244 to d06a6ab Compare October 27, 2016 09:14
@zhenlineo zhenlineo changed the title Refactoring on the connection pool Routing driver Oct 27, 2016
@zhenlineo zhenlineo force-pushed the 1.1-discovery-and-routing branch from d06a6ab to d143b54 Compare October 27, 2016 11:46
Zhen Li added 2 commits October 27, 2016 16:46
Built a `MockedMessagingClient` to verify messages sent via driver and to simulate messages from the server for tests
Added tests to verify error handlers are added correctly
Fixed the bug where the error handlers are added after client.start and init.
@zhenlineo zhenlineo force-pushed the 1.1-discovery-and-routing branch from fd97a8b to 898dea3 Compare November 2, 2016 12:32
Copy link
Contributor

@lutovich lutovich left a comment

Choose a reason for hiding this comment

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

@zhenlineo changes look good to me

{
private const int MinRouterCount = 1;
private readonly ConcurrentRoundRobinSet<Uri> _routers = new ConcurrentRoundRobinSet<Uri>();
private readonly ConcurrentRoundRobinSet<Uri> _detachedRouters = new ConcurrentRoundRobinSet<Uri>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Uris are only added to this set, otherwise it is never used. We need to decide if _detachedRouters is needed.


public static bool IsClusterError(this Neo4jException error)
{
return error.Code.Equals("Neo.ClientError.Cluster.NotALeader")
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to have constants or helper methods for these strings Neo.ClientError.Cluster.NotALeader and Neo.ClientError.General.ForbiddenOnReadOnlyDatabase. They are used here in Neo4jErrorExtensions and in RoundRobinLoadBalancer.


if (uri.Port == -1)
{
var builder = new UriBuilder(uri.Scheme, uri.Host, 7687);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's introduce a constant for 7687

@zhenlineo zhenlineo merged commit 211cc3c into neo4j:1.1 Nov 11, 2016
@zhenlineo zhenlineo deleted the 1.1-discovery-and-routing branch January 24, 2017 09:35
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.

2 participants