From ab54cc5b4efe53f737b76eda6f81b6d3ed9eb32b Mon Sep 17 00:00:00 2001 From: lutovich Date: Tue, 4 Jul 2017 12:28:26 +0200 Subject: [PATCH 01/10] Decouple load balancing logic from address set Round-robin load balancing was previously coupled with the address set implementation and made it hard to use different load balancing algorithm. This commit turns RoundRobinAddressSet into a simple concurrent set of addresses and moves load balancing logic into a separate component that takes list of available addresses and returns one that should be used next. This allows easier implementation of new load balancing algorithms. Rediscovery procedure will now not use load balancing and query routers in a known order (which is randomized by the database). --- ...ndRobinAddressSet.java => AddressSet.java} | 36 +-- .../internal/cluster/ClusterRoutingTable.java | 26 +- .../driver/internal/cluster/LoadBalancer.java | 32 ++- .../cluster/LoadBalancingStrategy.java | 28 +++ .../driver/internal/cluster/Rediscovery.java | 11 +- .../cluster/RoundRobinArrayIndex.java | 53 ++++ .../RoundRobinLoadBalancingStrategy.java | 50 ++++ .../driver/internal/cluster/RoutingTable.java | 8 +- .../internal/RoutingDriverBoltKitTest.java | 10 +- .../internal/cluster/AddressSetTest.java | 171 +++++++++++++ .../cluster/ClusterRoutingTableTest.java | 24 +- .../internal/cluster/LoadBalancerTest.java | 22 +- .../internal/cluster/RediscoveryTest.java | 48 ++++ .../cluster/RoundRobinAddressSetTest.java | 226 ------------------ .../cluster/RoundRobinArrayIndexTest.java | 67 ++++++ .../RoundRobinLoadBalancingStrategyTest.java | 98 ++++++++ .../neo4j/driver/internal/util/Matchers.java | 15 +- 17 files changed, 595 insertions(+), 330 deletions(-) rename driver/src/main/java/org/neo4j/driver/internal/cluster/{RoundRobinAddressSet.java => AddressSet.java} (78%) create mode 100644 driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancingStrategy.java create mode 100644 driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndex.java create mode 100644 driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategy.java create mode 100644 driver/src/test/java/org/neo4j/driver/internal/cluster/AddressSetTest.java delete mode 100644 driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinAddressSetTest.java create mode 100644 driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndexTest.java create mode 100644 driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategyTest.java diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinAddressSet.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/AddressSet.java similarity index 78% rename from driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinAddressSet.java rename to driver/src/main/java/org/neo4j/driver/internal/cluster/AddressSet.java index b933f6da5c..2bd03b5d31 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinAddressSet.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/AddressSet.java @@ -20,39 +20,23 @@ import java.util.Arrays; import java.util.Set; -import java.util.concurrent.atomic.AtomicInteger; import org.neo4j.driver.internal.net.BoltServerAddress; -public class RoundRobinAddressSet +public class AddressSet { private static final BoltServerAddress[] NONE = {}; - private final AtomicInteger offset = new AtomicInteger(); - private volatile BoltServerAddress[] addresses = NONE; - public int size() - { - return addresses.length; - } + private volatile BoltServerAddress[] addresses = NONE; - public BoltServerAddress next() + public BoltServerAddress[] toArray() { - BoltServerAddress[] addresses = this.addresses; - if ( addresses.length == 0 ) - { - return null; - } - return addresses[next( addresses.length )]; + return addresses; } - int next( int divisor ) + public int size() { - int index = offset.getAndIncrement(); - for ( ; index == Integer.MAX_VALUE; index = offset.getAndIncrement() ) - { - offset.compareAndSet( Integer.MIN_VALUE, index % divisor ); - } - return index % divisor; + return addresses.length; } public synchronized void update( Set addresses, Set removed ) @@ -132,12 +116,6 @@ public synchronized void remove( BoltServerAddress address ) @Override public String toString() { - return "RoundRobinAddressSet=" + Arrays.toString( addresses ); - } - - /** breaking encapsulation in order to perform white-box testing of boundary case */ - void setOffset( int target ) - { - offset.set( target ); + return "AddressSet=" + Arrays.toString( addresses ); } } diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterRoutingTable.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterRoutingTable.java index 54a80d60d7..36812ad945 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterRoutingTable.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterRoutingTable.java @@ -36,9 +36,9 @@ public class ClusterRoutingTable implements RoutingTable private final Clock clock; private volatile long expirationTimeout; - private final RoundRobinAddressSet readers; - private final RoundRobinAddressSet writers; - private final RoundRobinAddressSet routers; + private final AddressSet readers; + private final AddressSet writers; + private final AddressSet routers; public ClusterRoutingTable( Clock clock, BoltServerAddress... routingAddresses ) { @@ -51,9 +51,9 @@ private ClusterRoutingTable( Clock clock ) this.clock = clock; this.expirationTimeout = clock.millis() - 1; - this.readers = new RoundRobinAddressSet(); - this.writers = new RoundRobinAddressSet(); - this.routers = new RoundRobinAddressSet(); + this.readers = new AddressSet(); + this.writers = new AddressSet(); + this.routers = new AddressSet(); } @Override @@ -85,27 +85,21 @@ public synchronized void forget( BoltServerAddress address ) } @Override - public RoundRobinAddressSet readers() + public AddressSet readers() { return readers; } @Override - public RoundRobinAddressSet writers() + public AddressSet writers() { return writers; } @Override - public BoltServerAddress nextRouter() + public AddressSet routers() { - return routers.next(); - } - - @Override - public int routerSize() - { - return routers.size(); + return routers; } @Override diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java index ef48041d29..b46fba3111 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java @@ -39,6 +39,7 @@ public class LoadBalancer implements ConnectionProvider, RoutingErrorHandler, Au private final ConnectionPool connections; private final RoutingTable routingTable; private final Rediscovery rediscovery; + private final LoadBalancingStrategy loadBalancingStrategy; private final Logger log; public LoadBalancer( BoltServerAddress initialRouter, RoutingSettings settings, ConnectionPool connections, @@ -59,6 +60,7 @@ private LoadBalancer( BoltServerAddress initialRouter, RoutingSettings settings, this.connections = connections; this.routingTable = routingTable; this.rediscovery = rediscovery; + this.loadBalancingStrategy = new RoundRobinLoadBalancingStrategy(); this.log = log; refreshRoutingTable(); @@ -67,7 +69,7 @@ private LoadBalancer( BoltServerAddress initialRouter, RoutingSettings settings, @Override public PooledConnection acquireConnection( AccessMode mode ) { - RoundRobinAddressSet addressSet = addressSetFor( mode ); + AddressSet addressSet = addressSetFor( mode ); PooledConnection connection = acquireConnection( mode, addressSet ); return new RoutingPooledConnection( connection, this, mode ); } @@ -90,10 +92,10 @@ public void close() throws Exception connections.close(); } - private PooledConnection acquireConnection( AccessMode mode, RoundRobinAddressSet servers ) + private PooledConnection acquireConnection( AccessMode mode, AddressSet servers ) { ensureRouting( mode ); - for ( BoltServerAddress address; (address = servers.next()) != null; ) + for ( BoltServerAddress address; (address = selectAddress( mode, servers )) != null; ) { try { @@ -141,7 +143,7 @@ synchronized void refreshRoutingTable() log.info( "Refreshed routing information. %s", routingTable ); } - private RoundRobinAddressSet addressSetFor( AccessMode mode ) + private AddressSet addressSetFor( AccessMode mode ) { switch ( mode ) { @@ -150,7 +152,22 @@ private RoundRobinAddressSet addressSetFor( AccessMode mode ) case WRITE: return routingTable.writers(); default: - throw new IllegalArgumentException( "Mode '" + mode + "' is not supported" ); + throw unknownMode( mode ); + } + } + + private BoltServerAddress selectAddress( AccessMode mode, AddressSet servers ) + { + BoltServerAddress[] addresses = servers.toArray(); + + switch ( mode ) + { + case READ: + return loadBalancingStrategy.selectReader( addresses ); + case WRITE: + return loadBalancingStrategy.selectWriter( addresses ); + default: + throw unknownMode( mode ); } } @@ -161,4 +178,9 @@ private static Rediscovery createRediscovery( BoltServerAddress initialRouter, R new RoutingProcedureClusterCompositionProvider( clock, log, settings ); return new Rediscovery( initialRouter, settings, clock, log, clusterComposition, new DnsResolver( log ) ); } + + private static RuntimeException unknownMode( AccessMode mode ) + { + return new IllegalArgumentException( "Mode '" + mode + "' is not supported" ); + } } diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancingStrategy.java new file mode 100644 index 0000000000..99f40fcd42 --- /dev/null +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancingStrategy.java @@ -0,0 +1,28 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal.cluster; + +import org.neo4j.driver.internal.net.BoltServerAddress; + +public interface LoadBalancingStrategy +{ + BoltServerAddress selectReader( BoltServerAddress[] knownReaders ); + + BoltServerAddress selectWriter( BoltServerAddress[] knownWriters ); +} diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/Rediscovery.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/Rediscovery.java index a038da4dfc..de4db16771 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/Rediscovery.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/Rediscovery.java @@ -136,15 +136,10 @@ private ClusterComposition lookupOnInitialRouterThenOnKnownRouters( RoutingTable private ClusterComposition lookupOnKnownRouters( RoutingTable routingTable, ConnectionPool connections, Set seenServers ) { - int size = routingTable.routerSize(); - for ( int i = 0; i < size; i++ ) - { - BoltServerAddress address = routingTable.nextRouter(); - if ( address == null ) - { - break; - } + BoltServerAddress[] addresses = routingTable.routers().toArray(); + for ( BoltServerAddress address : addresses ) + { ClusterComposition composition = lookupOnRouter( address, routingTable, connections ); if ( composition != null ) { diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndex.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndex.java new file mode 100644 index 0000000000..cc1002a875 --- /dev/null +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndex.java @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal.cluster; + +import java.util.concurrent.atomic.AtomicInteger; + +public class RoundRobinArrayIndex +{ + private final AtomicInteger offset; + + RoundRobinArrayIndex() + { + this( 0 ); + } + + // only for testing + RoundRobinArrayIndex( int initialOffset ) + { + this.offset = new AtomicInteger( initialOffset ); + } + + public int next( int arrayLength ) + { + if ( arrayLength == 0 ) + { + return -1; + } + + int nextOffset; + while ( (nextOffset = offset.getAndIncrement()) < 0 ) + { + // overflow, try resetting back to zero + offset.compareAndSet( nextOffset + 1, 0 ); + } + return nextOffset % arrayLength; + } +} diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategy.java new file mode 100644 index 0000000000..35cb127b85 --- /dev/null +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategy.java @@ -0,0 +1,50 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal.cluster; + +import org.neo4j.driver.internal.net.BoltServerAddress; + +public class RoundRobinLoadBalancingStrategy implements LoadBalancingStrategy +{ + private final RoundRobinArrayIndex readersIndex = new RoundRobinArrayIndex(); + private final RoundRobinArrayIndex writersIndex = new RoundRobinArrayIndex(); + + @Override + public BoltServerAddress selectReader( BoltServerAddress[] knownReaders ) + { + return select( knownReaders, readersIndex ); + } + + @Override + public BoltServerAddress selectWriter( BoltServerAddress[] knownWriters ) + { + return select( knownWriters, writersIndex ); + } + + private BoltServerAddress select( BoltServerAddress[] addresses, RoundRobinArrayIndex roundRobinIndex ) + { + int length = addresses.length; + if ( length == 0 ) + { + return null; + } + int index = roundRobinIndex.next( length ); + return addresses[index]; + } +} diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoutingTable.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/RoutingTable.java index 9ec32dddb0..162ab7a51c 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoutingTable.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/RoutingTable.java @@ -31,13 +31,11 @@ public interface RoutingTable void forget( BoltServerAddress address ); - RoundRobinAddressSet readers(); + AddressSet readers(); - RoundRobinAddressSet writers(); + AddressSet writers(); - BoltServerAddress nextRouter(); - - int routerSize(); + AddressSet routers(); void removeWriter( BoltServerAddress toRemove ); } diff --git a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverBoltKitTest.java b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverBoltKitTest.java index 8c6ea1f5a3..920b53a71c 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverBoltKitTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverBoltKitTest.java @@ -808,11 +808,11 @@ public void shouldRetryReadTransactionAndPerformRediscoveryUntilSuccess() throws @Test public void shouldRetryWriteTransactionAndPerformRediscoveryUntilSuccess() throws Exception { - StubServer router1 = StubServer.start( "acquire_endpoints.script", 9010 ); - StubServer brokenWriter1 = StubServer.start( "dead_write_server.script", 9007 ); + StubServer router1 = StubServer.start( "discover_servers.script", 9010 ); + StubServer brokenWriter1 = StubServer.start( "dead_write_server.script", 9001 ); + StubServer router2 = StubServer.start( "acquire_endpoints.script", 9002 ); StubServer brokenWriter2 = StubServer.start( "dead_write_server.script", 9008 ); - StubServer router2 = StubServer.start( "discover_servers.script", 9002 ); - StubServer writer = StubServer.start( "write_server.script", 9001 ); + StubServer writer = StubServer.start( "write_server.script", 9007 ); try ( Driver driver = newDriverWithSleeplessClock( "bolt+routing://127.0.0.1:9010" ); Session session = driver.session() ) @@ -827,9 +827,9 @@ public void shouldRetryWriteTransactionAndPerformRediscoveryUntilSuccess() throw { assertEquals( 0, router1.exitStatus() ); assertEquals( 0, brokenWriter1.exitStatus() ); - assertEquals( 0, brokenWriter2.exitStatus() ); assertEquals( 0, router2.exitStatus() ); assertEquals( 0, writer.exitStatus() ); + assertEquals( 0, brokenWriter2.exitStatus() ); } } diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/AddressSetTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/AddressSetTest.java new file mode 100644 index 0000000000..b8af2751a5 --- /dev/null +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/AddressSetTest.java @@ -0,0 +1,171 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal.cluster; + +import org.junit.Test; + +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.Set; + +import org.neo4j.driver.internal.net.BoltServerAddress; + +import static java.util.Collections.singleton; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; + +public class AddressSetTest +{ + @Test + public void shouldPreserveOrderWhenAdding() throws Exception + { + // given + Set servers = addresses( "one", "two", "tre" ); + + AddressSet set = new AddressSet(); + set.update( servers, new HashSet() ); + + assertArrayEquals( new BoltServerAddress[]{ + new BoltServerAddress( "one" ), + new BoltServerAddress( "two" ), + new BoltServerAddress( "tre" )}, set.toArray() ); + + // when + servers.add( new BoltServerAddress( "fyr" ) ); + set.update( servers, new HashSet() ); + + // then + assertArrayEquals( new BoltServerAddress[]{ + new BoltServerAddress( "one" ), + new BoltServerAddress( "two" ), + new BoltServerAddress( "tre" ), + new BoltServerAddress( "fyr" )}, set.toArray() ); + } + + @Test + public void shouldPreserveOrderWhenRemoving() throws Exception + { + // given + Set servers = addresses( "one", "two", "tre" ); + AddressSet set = new AddressSet(); + set.update( servers, new HashSet() ); + + assertArrayEquals( new BoltServerAddress[]{ + new BoltServerAddress( "one" ), + new BoltServerAddress( "two" ), + new BoltServerAddress( "tre" )}, set.toArray() ); + + // when + set.remove( new BoltServerAddress( "one" ) ); + + // then + assertArrayEquals( new BoltServerAddress[]{ + new BoltServerAddress( "two" ), + new BoltServerAddress( "tre" )}, set.toArray() ); + } + + @Test + public void shouldPreserveOrderWhenRemovingThroughUpdate() throws Exception + { + // given + Set servers = addresses( "one", "two", "tre" ); + AddressSet set = new AddressSet(); + set.update( servers, new HashSet() ); + + assertArrayEquals( new BoltServerAddress[]{ + new BoltServerAddress( "one" ), + new BoltServerAddress( "two" ), + new BoltServerAddress( "tre" )}, set.toArray() ); + + // when + servers.remove( new BoltServerAddress( "one" ) ); + set.update( servers, new HashSet() ); + + // then + assertArrayEquals( new BoltServerAddress[]{ + new BoltServerAddress( "two" ), + new BoltServerAddress( "tre" )}, set.toArray() ); + } + + @Test + public void shouldRecordRemovedAddressesWhenUpdating() throws Exception + { + // given + AddressSet set = new AddressSet(); + set.update( addresses( "one", "two", "tre" ), new HashSet() ); + + // when + HashSet removed = new HashSet<>(); + set.update( addresses( "one", "two", "fyr" ), removed ); + + // then + assertEquals( singleton( new BoltServerAddress( "tre" ) ), removed ); + } + + @Test + public void shouldExposeEmptyArrayWhenEmpty() + { + AddressSet addressSet = new AddressSet(); + + BoltServerAddress[] addresses = addressSet.toArray(); + + assertEquals( 0, addresses.length ); + } + + @Test + public void shouldExposeCorrectArray() + { + AddressSet addressSet = new AddressSet(); + addressSet.update( addresses( "one", "two", "tre" ), new HashSet() ); + + BoltServerAddress[] addresses = addressSet.toArray(); + + assertArrayEquals( new BoltServerAddress[]{ + new BoltServerAddress( "one" ), + new BoltServerAddress( "two" ), + new BoltServerAddress( "tre" )}, addresses ); + } + + @Test + public void shouldHaveSizeZeroWhenEmpty() + { + AddressSet addressSet = new AddressSet(); + + assertEquals( 0, addressSet.size() ); + } + + @Test + public void shouldHaveCorrectSize() + { + AddressSet addressSet = new AddressSet(); + addressSet.update( addresses( "one", "two" ), new HashSet() ); + + assertEquals( 2, addressSet.size() ); + } + + private static Set addresses( String... strings ) + { + Set set = new LinkedHashSet<>(); + for ( String string : strings ) + { + set.add( new BoltServerAddress( string ) ); + } + return set; + } +} diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/ClusterRoutingTableTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/ClusterRoutingTableTest.java index 0a12c079cf..2766990516 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/ClusterRoutingTableTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/ClusterRoutingTableTest.java @@ -27,7 +27,7 @@ import static java.util.Arrays.asList; import static java.util.Collections.singletonList; -import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.neo4j.driver.internal.cluster.ClusterCompositionUtil.A; @@ -142,13 +142,7 @@ public void shouldPreserveOrderingOfRouters() routingTable.update( createClusterComposition( routers, EMPTY, EMPTY ) ); - assertEquals( A, routingTable.nextRouter() ); - assertEquals( C, routingTable.nextRouter() ); - assertEquals( D, routingTable.nextRouter() ); - assertEquals( F, routingTable.nextRouter() ); - assertEquals( B, routingTable.nextRouter() ); - assertEquals( E, routingTable.nextRouter() ); - assertEquals( A, routingTable.nextRouter() ); + assertArrayEquals( new BoltServerAddress[]{A, C, D, F, B, E}, routingTable.routers().toArray() ); } @Test @@ -159,12 +153,7 @@ public void shouldPreserveOrderingOfWriters() routingTable.update( createClusterComposition( EMPTY, writers, EMPTY ) ); - assertEquals( D, routingTable.writers().next() ); - assertEquals( F, routingTable.writers().next() ); - assertEquals( A, routingTable.writers().next() ); - assertEquals( C, routingTable.writers().next() ); - assertEquals( E, routingTable.writers().next() ); - assertEquals( D, routingTable.writers().next() ); + assertArrayEquals( new BoltServerAddress[]{D, F, A, C, E}, routingTable.writers().toArray() ); } @Test @@ -175,12 +164,7 @@ public void shouldPreserveOrderingOfReaders() routingTable.update( createClusterComposition( EMPTY, EMPTY, readers ) ); - assertEquals( B, routingTable.readers().next() ); - assertEquals( A, routingTable.readers().next() ); - assertEquals( F, routingTable.readers().next() ); - assertEquals( C, routingTable.readers().next() ); - assertEquals( D, routingTable.readers().next() ); - assertEquals( B, routingTable.readers().next() ); + assertArrayEquals( new BoltServerAddress[]{B, A, F, C, D}, routingTable.readers().toArray() ); } @Test diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java index 873781b9a9..b787b2356a 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java @@ -53,7 +53,6 @@ import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; -import static org.mockito.Mockito.RETURNS_MOCKS; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.inOrder; @@ -176,9 +175,12 @@ public void shouldForgetAddressAndItsConnectionsOnServiceUnavailableWhileClosing @Test public void shouldForgetAddressAndItsConnectionsOnServiceUnavailableWhileClosingSession() { - RoutingTable routingTable = mock( RoutingTable.class, RETURNS_MOCKS ); - ConnectionPool connectionPool = mock( ConnectionPool.class ); BoltServerAddress address = new BoltServerAddress( "host", 42 ); + RoutingTable routingTable = mock( RoutingTable.class ); + AddressSet addressSet = mock( AddressSet.class ); + when( addressSet.toArray() ).thenReturn( new BoltServerAddress[]{address} ); + when( routingTable.writers() ).thenReturn( addressSet ); + ConnectionPool connectionPool = mock( ConnectionPool.class ); PooledConnection connectionWithFailingSync = newConnectionWithFailingSync( address ); when( connectionPool.acquire( any( BoltServerAddress.class ) ) ).thenReturn( connectionWithFailingSync ); Rediscovery rediscovery = mock( Rediscovery.class ); @@ -225,8 +227,8 @@ public void shouldThrowWhenRediscoveryReturnsNoSuitableServers() RoutingTable routingTable = mock( RoutingTable.class ); when( routingTable.isStaleFor( any( AccessMode.class ) ) ).thenReturn( true ); Rediscovery rediscovery = mock( Rediscovery.class ); - when( routingTable.readers() ).thenReturn( new RoundRobinAddressSet() ); - when( routingTable.writers() ).thenReturn( new RoundRobinAddressSet() ); + when( routingTable.readers() ).thenReturn( new AddressSet() ); + when( routingTable.writers() ).thenReturn( new AddressSet() ); LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER ); @@ -300,11 +302,11 @@ private LoadBalancer setupLoadBalancer( PooledConnection writerConn, PooledConne when( connPool.acquire( writer ) ).thenReturn( writerConn ); when( connPool.acquire( reader ) ).thenReturn( readConn ); - RoundRobinAddressSet writerAddrs = mock( RoundRobinAddressSet.class ); - when( writerAddrs.next() ).thenReturn( writer ); + AddressSet writerAddrs = mock( AddressSet.class ); + when( writerAddrs.toArray() ).thenReturn( new BoltServerAddress[]{writer} ); - RoundRobinAddressSet readerAddrs = mock( RoundRobinAddressSet.class ); - when( readerAddrs.next() ).thenReturn( reader ); + AddressSet readerAddrs = mock( AddressSet.class ); + when( readerAddrs.toArray() ).thenReturn( new BoltServerAddress[]{reader} ); RoutingTable routingTable = mock( RoutingTable.class ); when( routingTable.readers() ).thenReturn( readerAddrs ); @@ -336,7 +338,7 @@ private static RoutingTable newStaleRoutingTableMock( AccessMode mode ) when( routingTable.isStaleFor( mode ) ).thenReturn( true ); when( routingTable.update( any( ClusterComposition.class ) ) ).thenReturn( new HashSet() ); - RoundRobinAddressSet addresses = new RoundRobinAddressSet(); + AddressSet addresses = new AddressSet(); addresses.update( new HashSet<>( singletonList( LOCAL_DEFAULT ) ), new HashSet() ); when( routingTable.readers() ).thenReturn( addresses ); when( routingTable.writers() ).thenReturn( addresses ); diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/RediscoveryTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/RediscoveryTest.java index b204335145..33966304db 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/RediscoveryTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/RediscoveryTest.java @@ -23,6 +23,7 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameters; +import org.mockito.InOrder; import java.util.ArrayList; import java.util.Collection; @@ -49,6 +50,7 @@ import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; +import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -451,6 +453,52 @@ public void shouldNotUseInitialRouterTwiceIfRoutingTableContainsIt() } } + public static class KnownRoutersTest + { + @Test + public void shouldProbeAllKnownRoutersInOrder() + { + PooledConnection brokenConnection1 = mock( PooledConnection.class ); + PooledConnection goodConnection = mock( PooledConnection.class ); + PooledConnection brokenConnection2 = mock( PooledConnection.class ); + + ConnectionPool connections = mock( ConnectionPool.class ); + when( connections.acquire( A ) ).thenReturn( brokenConnection1 ); + when( connections.acquire( B ) ).thenReturn( goodConnection ); + when( connections.acquire( C ) ).thenReturn( brokenConnection2 ); + + ClusterCompositionProvider clusterComposition = mock( ClusterCompositionProvider.class ); + when( clusterComposition.getClusterComposition( brokenConnection1 ) ) + .thenThrow( new ServiceUnavailableException( "Can't connect" ) ); + when( clusterComposition.getClusterComposition( goodConnection ) ) + .thenReturn( success( VALID_CLUSTER_COMPOSITION ) ); + when( clusterComposition.getClusterComposition( brokenConnection2 ) ) + .thenThrow( new ServiceUnavailableException( "Can't connect" ) ); + + RoutingTable routingTable = new TestRoutingTable( A, B, C ); + + RoutingSettings settings = new RoutingSettings( 1, 0, null ); + Clock mockedClock = mock( Clock.class ); + Logger mockedLogger = mock( Logger.class ); + + Rediscovery rediscovery = new Rediscovery( A, settings, mockedClock, mockedLogger, clusterComposition, + directMapProvider ); + + ClusterComposition composition1 = rediscovery.lookupClusterComposition( routingTable, connections ); + assertEquals( VALID_CLUSTER_COMPOSITION, composition1 ); + + ClusterComposition composition2 = rediscovery.lookupClusterComposition( routingTable, connections ); + assertEquals( VALID_CLUSTER_COMPOSITION, composition2 ); + + // server A should've been removed after an unsuccessful attempt + InOrder inOrder = inOrder( clusterComposition ); + inOrder.verify( clusterComposition ).getClusterComposition( brokenConnection1 ); + inOrder.verify( clusterComposition, times( 2 ) ).getClusterComposition( goodConnection ); + + verify( clusterComposition, never() ).getClusterComposition( brokenConnection2 ); + } + } + private static ClusterComposition rediscover( ConnectionPool connections, RoutingTable routingTable, ClusterCompositionProvider provider ) { diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinAddressSetTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinAddressSetTest.java deleted file mode 100644 index 390ae234ec..0000000000 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinAddressSetTest.java +++ /dev/null @@ -1,226 +0,0 @@ -/* - * Copyright (c) 2002-2017 "Neo Technology," - * Network Engine for Objects in Lund AB [http://neotechnology.com] - * - * This file is part of Neo4j. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.neo4j.driver.internal.cluster; - -import java.util.ArrayList; -import java.util.HashSet; -import java.util.List; - -import org.junit.Test; - -import org.neo4j.driver.internal.net.BoltServerAddress; - -import static java.util.Arrays.asList; -import static java.util.Collections.singleton; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.fail; - -public class RoundRobinAddressSetTest -{ - @Test - public void shouldReturnNullWhenEmpty() throws Exception - { - // given - RoundRobinAddressSet set = new RoundRobinAddressSet(); - - // then - assertNull( set.next() ); - } - - @Test - public void shouldReturnRoundRobin() throws Exception - { - // given - RoundRobinAddressSet set = new RoundRobinAddressSet(); - set.update( new HashSet<>( asList( - new BoltServerAddress( "one" ), - new BoltServerAddress( "two" ), - new BoltServerAddress( "tre" ) ) ), new HashSet() ); - - // when - BoltServerAddress a = set.next(); - BoltServerAddress b = set.next(); - BoltServerAddress c = set.next(); - - // then - assertEquals( a, set.next() ); - assertEquals( b, set.next() ); - assertEquals( c, set.next() ); - assertEquals( a, set.next() ); - assertEquals( b, set.next() ); - assertEquals( c, set.next() ); - assertNotEquals( a, c ); - assertNotEquals( b, a ); - assertNotEquals( c, b ); - } - - @Test - public void shouldPreserveOrderWhenAdding() throws Exception - { - // given - HashSet servers = new HashSet<>( asList( - new BoltServerAddress( "one" ), - new BoltServerAddress( "two" ), - new BoltServerAddress( "tre" ) ) ); - RoundRobinAddressSet set = new RoundRobinAddressSet(); - set.update( servers, new HashSet() ); - - List order = new ArrayList<>(); - for ( int i = 3 * 4 + 1; i-- > 0; ) - { - BoltServerAddress server = set.next(); - if ( !order.contains( server ) ) - { - order.add( server ); - } - } - assertEquals( 3, order.size() ); - - // when - servers.add( new BoltServerAddress( "fyr" ) ); - set.update( servers, new HashSet() ); - - // then - assertEquals( order.get( 1 ), set.next() ); - assertEquals( order.get( 2 ), set.next() ); - BoltServerAddress next = set.next(); - assertNotEquals( order.get( 0 ), next ); - assertNotEquals( order.get( 1 ), next ); - assertNotEquals( order.get( 2 ), next ); - assertEquals( order.get( 0 ), set.next() ); - // ... and once more - assertEquals( order.get( 1 ), set.next() ); - assertEquals( order.get( 2 ), set.next() ); - assertEquals( next, set.next() ); - assertEquals( order.get( 0 ), set.next() ); - } - - @Test - public void shouldPreserveOrderWhenRemoving() throws Exception - { - // given - HashSet servers = new HashSet<>( asList( - new BoltServerAddress( "one" ), - new BoltServerAddress( "two" ), - new BoltServerAddress( "tre" ) ) ); - RoundRobinAddressSet set = new RoundRobinAddressSet(); - set.update( servers, new HashSet() ); - - List order = new ArrayList<>(); - for ( int i = 3 * 2 + 1; i-- > 0; ) - { - BoltServerAddress server = set.next(); - if ( !order.contains( server ) ) - { - order.add( server ); - } - } - assertEquals( 3, order.size() ); - - // when - set.remove( order.get( 1 ) ); - - // then - assertEquals( order.get( 2 ), set.next() ); - assertEquals( order.get( 0 ), set.next() ); - assertEquals( order.get( 2 ), set.next() ); - assertEquals( order.get( 0 ), set.next() ); - } - - @Test - public void shouldPreserveOrderWhenRemovingThroughUpdate() throws Exception - { - // given - HashSet servers = new HashSet<>( asList( - new BoltServerAddress( "one" ), - new BoltServerAddress( "two" ), - new BoltServerAddress( "tre" ) ) ); - RoundRobinAddressSet set = new RoundRobinAddressSet(); - set.update( servers, new HashSet() ); - - List order = new ArrayList<>(); - for ( int i = 3 * 2 + 1; i-- > 0; ) - { - BoltServerAddress server = set.next(); - if ( !order.contains( server ) ) - { - order.add( server ); - } - } - assertEquals( 3, order.size() ); - - // when - servers.remove( order.get( 1 ) ); - set.update( servers, new HashSet() ); - - // then - assertEquals( order.get( 2 ), set.next() ); - assertEquals( order.get( 0 ), set.next() ); - assertEquals( order.get( 2 ), set.next() ); - assertEquals( order.get( 0 ), set.next() ); - } - - @Test - public void shouldRecordRemovedAddressesWhenUpdating() throws Exception - { - // given - RoundRobinAddressSet set = new RoundRobinAddressSet(); - set.update( - new HashSet<>( asList( - new BoltServerAddress( "one" ), - new BoltServerAddress( "two" ), - new BoltServerAddress( "tre" ) ) ), - new HashSet() ); - - // when - HashSet removed = new HashSet<>(); - set.update( - new HashSet<>( asList( - new BoltServerAddress( "one" ), - new BoltServerAddress( "two" ), - new BoltServerAddress( "fyr" ) ) ), - removed ); - - // then - assertEquals( singleton( new BoltServerAddress( "tre" ) ), removed ); - } - - @Test - public void shouldPreserveOrderEvenWhenIntegerOverflows() throws Exception - { - // given - RoundRobinAddressSet set = new RoundRobinAddressSet(); - - for ( int div = 1; div <= 1024; div++ ) - { - // when - white box testing! - set.setOffset( Integer.MAX_VALUE - 1 ); - int a = set.next( div ); - int b = set.next( div ); - - // then - if ( b != (a + 1) % div ) - { - fail( String.format( "a=%d, b=%d, div=%d, (a+1)%%div=%d", a, b, div, (a + 1) % div ) ); - } - } - } -} diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndexTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndexTest.java new file mode 100644 index 0000000000..a91b678ca4 --- /dev/null +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndexTest.java @@ -0,0 +1,67 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal.cluster; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class RoundRobinArrayIndexTest +{ + @Test + public void shouldHandleZeroLength() + { + RoundRobinArrayIndex roundRobinIndex = new RoundRobinArrayIndex(); + + int index = roundRobinIndex.next( 0 ); + + assertEquals( -1, index ); + } + + @Test + public void shouldReturnIndexesInRoundRobinOrder() + { + RoundRobinArrayIndex roundRobinIndex = new RoundRobinArrayIndex(); + + for ( int i = 0; i < 10; i++ ) + { + int index = roundRobinIndex.next( 10 ); + assertEquals( i, index ); + } + + for ( int i = 0; i < 5; i++ ) + { + int index = roundRobinIndex.next( 5 ); + assertEquals( i, index ); + } + } + + @Test + public void shouldHandleOverflow() + { + int arrayLength = 10; + RoundRobinArrayIndex roundRobinIndex = new RoundRobinArrayIndex( Integer.MAX_VALUE - 1 ); + + assertEquals( (Integer.MAX_VALUE - 1) % arrayLength, roundRobinIndex.next( arrayLength ) ); + assertEquals( Integer.MAX_VALUE % arrayLength, roundRobinIndex.next( arrayLength ) ); + assertEquals( 0, roundRobinIndex.next( arrayLength ) ); + assertEquals( 1, roundRobinIndex.next( arrayLength ) ); + assertEquals( 2, roundRobinIndex.next( arrayLength ) ); + } +} diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategyTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategyTest.java new file mode 100644 index 0000000000..63ef0ad8e4 --- /dev/null +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategyTest.java @@ -0,0 +1,98 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal.cluster; + +import org.junit.Test; + +import org.neo4j.driver.internal.net.BoltServerAddress; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +public class RoundRobinLoadBalancingStrategyTest +{ + private final RoundRobinLoadBalancingStrategy strategy = new RoundRobinLoadBalancingStrategy(); + + @Test + public void shouldHandleEmptyReadersArray() + { + assertNull( strategy.selectReader( new BoltServerAddress[0] ) ); + } + + @Test + public void shouldHandleEmptyWritersArray() + { + assertNull( strategy.selectWriter( new BoltServerAddress[0] ) ); + } + + @Test + public void shouldHandleSingleReader() + { + BoltServerAddress address = new BoltServerAddress( "reader", 9999 ); + + assertEquals( address, strategy.selectReader( new BoltServerAddress[]{address} ) ); + } + + @Test + public void shouldHandleSingleWriter() + { + BoltServerAddress address = new BoltServerAddress( "writer", 9999 ); + + assertEquals( address, strategy.selectWriter( new BoltServerAddress[]{address} ) ); + } + + @Test + public void shouldReturnReadersInRoundRobinOrder() + { + BoltServerAddress address1 = new BoltServerAddress( "server-1", 1 ); + BoltServerAddress address2 = new BoltServerAddress( "server-2", 2 ); + BoltServerAddress address3 = new BoltServerAddress( "server-3", 3 ); + BoltServerAddress address4 = new BoltServerAddress( "server-4", 4 ); + + BoltServerAddress[] readers = {address1, address2, address3, address4}; + + assertEquals( address1, strategy.selectReader( readers ) ); + assertEquals( address2, strategy.selectReader( readers ) ); + assertEquals( address3, strategy.selectReader( readers ) ); + assertEquals( address4, strategy.selectReader( readers ) ); + + assertEquals( address1, strategy.selectReader( readers ) ); + assertEquals( address2, strategy.selectReader( readers ) ); + assertEquals( address3, strategy.selectReader( readers ) ); + assertEquals( address4, strategy.selectReader( readers ) ); + } + + @Test + public void shouldReturnWriterInRoundRobinOrder() + { + BoltServerAddress address1 = new BoltServerAddress( "server-1", 1 ); + BoltServerAddress address2 = new BoltServerAddress( "server-2", 2 ); + BoltServerAddress address3 = new BoltServerAddress( "server-3", 3 ); + + BoltServerAddress[] writers = {address1, address2, address3}; + + assertEquals( address1, strategy.selectWriter( writers ) ); + assertEquals( address2, strategy.selectWriter( writers ) ); + assertEquals( address3, strategy.selectWriter( writers ) ); + + assertEquals( address1, strategy.selectWriter( writers ) ); + assertEquals( address2, strategy.selectWriter( writers ) ); + assertEquals( address3, strategy.selectWriter( writers ) ); + } +} diff --git a/driver/src/test/java/org/neo4j/driver/internal/util/Matchers.java b/driver/src/test/java/org/neo4j/driver/internal/util/Matchers.java index 4433673370..a734dae5a1 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/util/Matchers.java +++ b/driver/src/test/java/org/neo4j/driver/internal/util/Matchers.java @@ -28,8 +28,8 @@ import org.neo4j.driver.internal.InternalDriver; import org.neo4j.driver.internal.SessionFactory; import org.neo4j.driver.internal.SessionFactoryImpl; +import org.neo4j.driver.internal.cluster.AddressSet; import org.neo4j.driver.internal.cluster.LoadBalancer; -import org.neo4j.driver.internal.cluster.RoundRobinAddressSet; import org.neo4j.driver.internal.cluster.RoutingTable; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.spi.ConnectionProvider; @@ -48,9 +48,11 @@ public static Matcher containsRouter( final BoltServerAddress addr @Override protected boolean matchesSafely( RoutingTable routingTable ) { - for ( int i = 0; i < routingTable.routerSize(); i++ ) + BoltServerAddress[] addresses = routingTable.routers().toArray(); + + for ( BoltServerAddress currentAddress : addresses ) { - if ( routingTable.nextRouter().equals( address ) ) + if ( currentAddress.equals( address ) ) { return true; } @@ -157,11 +159,12 @@ public void describeTo( Description description ) }; } - private static boolean contains( RoundRobinAddressSet set, BoltServerAddress address ) + private static boolean contains( AddressSet set, BoltServerAddress address ) { - for ( int i = 0; i < set.size(); i++ ) + BoltServerAddress[] addresses = set.toArray(); + for ( BoltServerAddress currentAddress : addresses ) { - if ( set.next().equals( address ) ) + if ( currentAddress.equals( address ) ) { return true; } From 4357602b01d6104ac63db74d6de2f417deb30261 Mon Sep 17 00:00:00 2001 From: lutovich Date: Wed, 5 Jul 2017 16:38:42 +0200 Subject: [PATCH 02/10] Added least connected load balancing strategy This replaces the existing round-robin. It gives us better performance with clusters composed of different machine capacities. Lease connected load balancing strategy selects start index in round-robin fashion to avoid always selecting same machine when cluster does not have any running transactions or all machines have same number of active connections. --- .../LeastConnectedLoadBalancingStrategy.java | 88 ++++++++++ .../driver/internal/cluster/LoadBalancer.java | 2 +- .../BlockingPooledConnectionQueue.java | 5 + .../net/pooling/SocketConnectionPool.java | 7 + .../driver/internal/spi/ConnectionPool.java | 18 ++- ...astConnectedLoadBalancingStrategyTest.java | 151 ++++++++++++++++++ .../internal/cluster/LoadBalancerTest.java | 79 +++++++++ .../BlockingPooledConnectionQueueTest.java | 46 ++++++ .../net/pooling/SocketConnectionPoolTest.java | 47 ++++++ 9 files changed, 440 insertions(+), 3 deletions(-) create mode 100644 driver/src/main/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategy.java create mode 100644 driver/src/test/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategyTest.java diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategy.java new file mode 100644 index 0000000000..030333b2ed --- /dev/null +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategy.java @@ -0,0 +1,88 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal.cluster; + +import org.neo4j.driver.internal.net.BoltServerAddress; +import org.neo4j.driver.internal.spi.ConnectionPool; + +public class LeastConnectedLoadBalancingStrategy implements LoadBalancingStrategy +{ + private final RoundRobinArrayIndex readersIndex = new RoundRobinArrayIndex(); + private final RoundRobinArrayIndex writersIndex = new RoundRobinArrayIndex(); + + private final ConnectionPool connectionPool; + + public LeastConnectedLoadBalancingStrategy( ConnectionPool connectionPool ) + { + this.connectionPool = connectionPool; + } + + @Override + public BoltServerAddress selectReader( BoltServerAddress[] knownReaders ) + { + return select( knownReaders, readersIndex ); + } + + @Override + public BoltServerAddress selectWriter( BoltServerAddress[] knownWriters ) + { + return select( knownWriters, writersIndex ); + } + + private BoltServerAddress select( BoltServerAddress[] addresses, RoundRobinArrayIndex addressesIndex ) + { + int size = addresses.length; + if ( size == 0 ) + { + return null; + } + else + { + int startIndex = addressesIndex.next( size ); + int index = startIndex; + + BoltServerAddress leastConnectedAddress = null; + int leastActiveConnections = Integer.MAX_VALUE; + + do + { + BoltServerAddress address = addresses[index]; + int activeConnections = connectionPool.activeConnections( address ); + + if ( activeConnections < leastActiveConnections ) + { + leastConnectedAddress = address; + leastActiveConnections = activeConnections; + } + + if ( index == size - 1 ) + { + index = 0; + } + else + { + index++; + } + } + while ( index != startIndex ); + + return leastConnectedAddress; + } + } +} diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java index b46fba3111..05c124303a 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java @@ -60,7 +60,7 @@ private LoadBalancer( BoltServerAddress initialRouter, RoutingSettings settings, this.connections = connections; this.routingTable = routingTable; this.rediscovery = rediscovery; - this.loadBalancingStrategy = new RoundRobinLoadBalancingStrategy(); + this.loadBalancingStrategy = new LeastConnectedLoadBalancingStrategy( connections ); this.log = log; refreshRoutingTable(); diff --git a/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java b/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java index a62f757aa5..89b90ad979 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java +++ b/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java @@ -120,6 +120,11 @@ public int size() return queue.size(); } + public int activeConnections() + { + return acquiredConnections.size(); + } + public boolean contains( PooledConnection pooledConnection ) { return queue.contains( pooledConnection ); diff --git a/driver/src/main/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPool.java b/driver/src/main/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPool.java index 463525ecaa..9449a6f402 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPool.java +++ b/driver/src/main/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPool.java @@ -95,6 +95,13 @@ public boolean hasAddress( BoltServerAddress address ) return pools.containsKey( address ); } + @Override + public int activeConnections( BoltServerAddress address ) + { + BlockingPooledConnectionQueue connectionQueue = pools.get( address ); + return connectionQueue == null ? 0 : connectionQueue.activeConnections(); + } + @Override public void close() { diff --git a/driver/src/main/java/org/neo4j/driver/internal/spi/ConnectionPool.java b/driver/src/main/java/org/neo4j/driver/internal/spi/ConnectionPool.java index 5ca7268176..8f98c2084e 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/spi/ConnectionPool.java +++ b/driver/src/main/java/org/neo4j/driver/internal/spi/ConnectionPool.java @@ -26,15 +26,29 @@ public interface ConnectionPool extends AutoCloseable * Acquire a connection - if a live connection exists in the pool, it will * be used, otherwise a new connection will be created. * - * @param address The address to acquire + * @param address the address to acquire */ PooledConnection acquire( BoltServerAddress address ); /** * Removes all connections to a given address from the pool. - * @param address The address to remove. + * @param address the address to remove. */ void purge( BoltServerAddress address ); + /** + * Check if pool has connections for the given address. + * + * @param address the address to check connections. + * @return {@code true} when pool has connections towards the given address, {@code false} otherwise. + */ boolean hasAddress( BoltServerAddress address ); + + /** + * Gen number of active connections pool has towards the given address. + * + * @param address the address to get connections. + * @return number of active (checked out of this pool) connections. + */ + int activeConnections( BoltServerAddress address ); } diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategyTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategyTest.java new file mode 100644 index 0000000000..57b46d513d --- /dev/null +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategyTest.java @@ -0,0 +1,151 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal.cluster; + +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mock; + +import org.neo4j.driver.internal.net.BoltServerAddress; +import org.neo4j.driver.internal.spi.ConnectionPool; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.mockito.Mockito.when; +import static org.mockito.MockitoAnnotations.initMocks; + +public class LeastConnectedLoadBalancingStrategyTest +{ + @Mock + private ConnectionPool connectionPool; + private LeastConnectedLoadBalancingStrategy strategy; + + @Before + public void setUp() throws Exception + { + initMocks( this ); + strategy = new LeastConnectedLoadBalancingStrategy( connectionPool ); + } + + @Test + public void shouldHandleEmptyReadersArray() + { + assertNull( strategy.selectReader( new BoltServerAddress[0] ) ); + } + + @Test + public void shouldHandleEmptyWritersArray() + { + assertNull( strategy.selectWriter( new BoltServerAddress[0] ) ); + } + + @Test + public void shouldHandleSingleReaderWithoutActiveConnections() + { + BoltServerAddress address = new BoltServerAddress( "reader", 9999 ); + + assertEquals( address, strategy.selectReader( new BoltServerAddress[]{address} ) ); + } + + @Test + public void shouldHandleSingleWriterWithoutActiveConnections() + { + BoltServerAddress address = new BoltServerAddress( "writer", 9999 ); + + assertEquals( address, strategy.selectWriter( new BoltServerAddress[]{address} ) ); + } + + @Test + public void shouldHandleSingleReaderWithActiveConnections() + { + BoltServerAddress address = new BoltServerAddress( "reader", 9999 ); + when( connectionPool.activeConnections( address ) ).thenReturn( 42 ); + + assertEquals( address, strategy.selectReader( new BoltServerAddress[]{address} ) ); + } + + @Test + public void shouldHandleSingleWriterWithActiveConnections() + { + BoltServerAddress address = new BoltServerAddress( "writer", 9999 ); + when( connectionPool.activeConnections( address ) ).thenReturn( 24 ); + + assertEquals( address, strategy.selectWriter( new BoltServerAddress[]{address} ) ); + } + + @Test + public void shouldHandleMultipleReadersWithActiveConnections() + { + BoltServerAddress address1 = new BoltServerAddress( "reader", 1 ); + BoltServerAddress address2 = new BoltServerAddress( "reader", 2 ); + BoltServerAddress address3 = new BoltServerAddress( "reader", 3 ); + + when( connectionPool.activeConnections( address1 ) ).thenReturn( 3 ); + when( connectionPool.activeConnections( address2 ) ).thenReturn( 4 ); + when( connectionPool.activeConnections( address3 ) ).thenReturn( 1 ); + + assertEquals( address3, strategy.selectReader( new BoltServerAddress[]{address1, address2, address3} ) ); + } + + @Test + public void shouldHandleMultipleWritersWithActiveConnections() + { + BoltServerAddress address1 = new BoltServerAddress( "writer", 1 ); + BoltServerAddress address2 = new BoltServerAddress( "writer", 2 ); + BoltServerAddress address3 = new BoltServerAddress( "writer", 3 ); + BoltServerAddress address4 = new BoltServerAddress( "writer", 4 ); + + when( connectionPool.activeConnections( address1 ) ).thenReturn( 5 ); + when( connectionPool.activeConnections( address2 ) ).thenReturn( 6 ); + when( connectionPool.activeConnections( address3 ) ).thenReturn( 0 ); + when( connectionPool.activeConnections( address4 ) ).thenReturn( 1 ); + + assertEquals( address3, + strategy.selectWriter( new BoltServerAddress[]{address1, address2, address3, address4} ) ); + } + + @Test + public void shouldReturnDifferentReaderOnEveryInvocationWhenNoActiveConnections() + { + BoltServerAddress address1 = new BoltServerAddress( "reader", 1 ); + BoltServerAddress address2 = new BoltServerAddress( "reader", 2 ); + BoltServerAddress address3 = new BoltServerAddress( "reader", 3 ); + + assertEquals( address1, strategy.selectReader( new BoltServerAddress[]{address1, address2, address3} ) ); + assertEquals( address2, strategy.selectReader( new BoltServerAddress[]{address1, address2, address3} ) ); + assertEquals( address3, strategy.selectReader( new BoltServerAddress[]{address1, address2, address3} ) ); + + assertEquals( address1, strategy.selectReader( new BoltServerAddress[]{address1, address2, address3} ) ); + assertEquals( address2, strategy.selectReader( new BoltServerAddress[]{address1, address2, address3} ) ); + assertEquals( address3, strategy.selectReader( new BoltServerAddress[]{address1, address2, address3} ) ); + } + + @Test + public void shouldReturnDifferentWriterOnEveryInvocationWhenNoActiveConnections() + { + BoltServerAddress address1 = new BoltServerAddress( "writer", 1 ); + BoltServerAddress address2 = new BoltServerAddress( "writer", 2 ); + + assertEquals( address1, strategy.selectReader( new BoltServerAddress[]{address1, address2} ) ); + assertEquals( address2, strategy.selectReader( new BoltServerAddress[]{address1, address2} ) ); + + assertEquals( address1, strategy.selectReader( new BoltServerAddress[]{address1, address2} ) ); + assertEquals( address2, strategy.selectReader( new BoltServerAddress[]{address1, address2} ) ); + } +} diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java index b787b2356a..2b2134548a 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java @@ -20,7 +20,10 @@ import org.junit.Test; import org.mockito.InOrder; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; +import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.Set; @@ -49,8 +52,10 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.startsWith; import static org.hamcrest.core.IsEqual.equalTo; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; import static org.mockito.Mockito.doReturn; @@ -61,6 +66,9 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.neo4j.driver.internal.cluster.ClusterCompositionUtil.A; +import static org.neo4j.driver.internal.cluster.ClusterCompositionUtil.B; +import static org.neo4j.driver.internal.cluster.ClusterCompositionUtil.C; import static org.neo4j.driver.internal.logging.DevNullLogger.DEV_NULL_LOGGER; import static org.neo4j.driver.internal.logging.DevNullLogging.DEV_NULL_LOGGING; import static org.neo4j.driver.internal.net.BoltServerAddress.LOCAL_DEFAULT; @@ -255,6 +263,60 @@ public void shouldThrowWhenRediscoveryReturnsNoSuitableServers() } } + @Test + public void shouldSelectLeastConnectedAddress() + { + ConnectionPool connectionPool = newConnectionPoolMock(); + when( connectionPool.activeConnections( A ) ).thenReturn( 0 ); + when( connectionPool.activeConnections( B ) ).thenReturn( 20 ); + when( connectionPool.activeConnections( C ) ).thenReturn( 0 ); + + RoutingTable routingTable = mock( RoutingTable.class ); + AddressSet readerAddresses = mock( AddressSet.class ); + when( readerAddresses.toArray() ).thenReturn( new BoltServerAddress[]{A, B, C} ); + when( routingTable.readers() ).thenReturn( readerAddresses ); + + Rediscovery rediscovery = mock( Rediscovery.class ); + + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + + Set seenAddresses = new HashSet<>(); + for ( int i = 0; i < 10; i++ ) + { + PooledConnection connection = loadBalancer.acquireConnection( READ ); + seenAddresses.add( connection.boltServerAddress() ); + } + + // server B should never be selected because it has many active connections + assertEquals( 2, seenAddresses.size() ); + assertTrue( seenAddresses.containsAll( Arrays.asList( A, C ) ) ); + } + + @Test + public void shouldRoundRobinWhenNoActiveConnections() + { + ConnectionPool connectionPool = newConnectionPoolMock(); + + RoutingTable routingTable = mock( RoutingTable.class ); + AddressSet readerAddresses = mock( AddressSet.class ); + when( readerAddresses.toArray() ).thenReturn( new BoltServerAddress[]{A, B, C} ); + when( routingTable.readers() ).thenReturn( readerAddresses ); + + Rediscovery rediscovery = mock( Rediscovery.class ); + + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + + Set seenAddresses = new HashSet<>(); + for ( int i = 0; i < 10; i++ ) + { + PooledConnection connection = loadBalancer.acquireConnection( READ ); + seenAddresses.add( connection.boltServerAddress() ); + } + + assertEquals( 3, seenAddresses.size() ); + assertTrue( seenAddresses.containsAll( Arrays.asList( A, B, C ) ) ); + } + private void testRediscoveryWhenStale( AccessMode mode ) { ConnectionPool connections = mock( ConnectionPool.class ); @@ -355,4 +417,21 @@ private static Rediscovery newRediscoveryMock() .thenReturn( clusterComposition ); return rediscovery; } + + private static ConnectionPool newConnectionPoolMock() + { + ConnectionPool connectionPool = mock( ConnectionPool.class ); + when( connectionPool.acquire( any( BoltServerAddress.class ) ) ).then( new Answer() + { + @Override + public PooledConnection answer( InvocationOnMock invocation ) throws Throwable + { + BoltServerAddress requestedAddress = invocation.getArgumentAt( 0, BoltServerAddress.class ); + PooledConnection connection = mock( PooledConnection.class ); + when( connection.boltServerAddress() ).thenReturn( requestedAddress ); + return connection; + } + } ); + return connectionPool; + } } diff --git a/driver/src/test/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueueTest.java b/driver/src/test/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueueTest.java index 67d6fad4bd..25f77a9b89 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueueTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueueTest.java @@ -239,6 +239,52 @@ public void shouldTerminateBothAcquiredAndIdleConnections() verify( connection4 ).dispose(); } + @Test + public void shouldReportZeroActiveConnectionsWhenEmpty() + { + BlockingPooledConnectionQueue queue = newConnectionQueue( 5 ); + + assertEquals( 0, queue.activeConnections() ); + } + + @Test + public void shouldReportZeroActiveConnectionsWhenHasOnlyIdleConnections() + { + BlockingPooledConnectionQueue queue = newConnectionQueue( 5 ); + + queue.offer( mock( PooledConnection.class ) ); + queue.offer( mock( PooledConnection.class ) ); + + assertEquals( 0, queue.activeConnections() ); + } + + @Test + @SuppressWarnings( "unchecked" ) + public void shouldReportActiveConnections() + { + BlockingPooledConnectionQueue queue = newConnectionQueue( 5 ); + + PooledConnection connection1 = mock( PooledConnection.class ); + PooledConnection connection2 = mock( PooledConnection.class ); + PooledConnection connection3 = mock( PooledConnection.class ); + + queue.offer( connection1 ); + queue.offer( connection2 ); + queue.offer( connection3 ); + + queue.acquire( mock( Supplier.class ) ); + queue.acquire( mock( Supplier.class ) ); + queue.acquire( mock( Supplier.class ) ); + + assertEquals( 3, queue.activeConnections() ); + + queue.offer( connection1 ); + queue.offer( connection2 ); + queue.offer( connection3 ); + + assertEquals( 0, queue.activeConnections() ); + } + private static BlockingPooledConnectionQueue newConnectionQueue( int capacity ) { return newConnectionQueue( capacity, mock( Logging.class, RETURNS_MOCKS ) ); diff --git a/driver/src/test/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPoolTest.java b/driver/src/test/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPoolTest.java index d761981469..bd3e96f1b7 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPoolTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPoolTest.java @@ -47,6 +47,7 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.isOneOf; import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertSame; @@ -489,6 +490,52 @@ public void acquireRetriesUntilAConnectionIsCreated() inOrder.verify( connection4, never() ).sync(); } + @Test + public void reportActiveConnectionsWhenEmpty() + { + SocketConnectionPool pool = newPool( newMockConnector() ); + + int activeConnections1 = pool.activeConnections( ADDRESS_1 ); + int activeConnections2 = pool.activeConnections( ADDRESS_2 ); + int activeConnections3 = pool.activeConnections( ADDRESS_3 ); + + assertEquals( 0, activeConnections1 ); + assertEquals( 0, activeConnections2 ); + assertEquals( 0, activeConnections3 ); + } + + @Test + public void reportActiveConnectionsWhenHasAcquiredConnections() + { + int acquiredConnections = 23; + SocketConnectionPool pool = newPool( newMockConnector() ); + + for ( int i = 0; i < acquiredConnections; i++ ) + { + assertNotNull( pool.acquire( ADDRESS_1 ) ); + } + + assertEquals( acquiredConnections, pool.activeConnections( ADDRESS_1 ) ); + } + + @Test + public void reportActiveConnectionsWhenHasIdleConnections() + { + Connection connection = newConnectionMock( ADDRESS_1 ); + Connector connector = newMockConnector( connection ); + SocketConnectionPool pool = newPool( connector ); + + PooledConnection connection1 = pool.acquire( ADDRESS_1 ); + PooledConnection connection2 = pool.acquire( ADDRESS_1 ); + + assertEquals( 2, pool.activeConnections( ADDRESS_1 ) ); + + connection1.close(); + connection2.close(); + + assertEquals( 0, pool.activeConnections( ADDRESS_1 ) ); + } + private static Answer createConnectionAnswer( final Set createdConnections ) { return new Answer() From 190ed2cb346011b6c3a84bd57d03e9f6e4da243d Mon Sep 17 00:00:00 2001 From: lutovich Date: Wed, 5 Jul 2017 17:14:08 +0200 Subject: [PATCH 03/10] Resturctured load balancers to a separate package --- .../org/neo4j/driver/internal/DriverFactory.java | 2 +- .../internal/cluster/ClusterComposition.java | 4 ++-- .../internal/cluster/RoutingPooledConnection.java | 4 ++-- .../LeastConnectedLoadBalancingStrategy.java | 2 +- .../cluster/{ => loadbalancing}/LoadBalancer.java | 15 +++++++++++++-- .../LoadBalancingStrategy.java | 2 +- .../{ => loadbalancing}/RoundRobinArrayIndex.java | 2 +- .../RoundRobinLoadBalancingStrategy.java | 2 +- .../neo4j/driver/internal/DriverFactoryTest.java | 2 +- .../neo4j/driver/internal/RoutingDriverTest.java | 2 +- .../RoutingPooledConnectionErrorHandlingTest.java | 1 + .../LeastConnectedLoadBalancingStrategyTest.java | 2 +- .../{ => loadbalancing}/LoadBalancerTest.java | 7 ++++++- .../RoundRobinArrayIndexTest.java | 2 +- .../RoundRobinLoadBalancingStrategyTest.java | 2 +- .../org/neo4j/driver/internal/util/Matchers.java | 2 +- 16 files changed, 35 insertions(+), 18 deletions(-) rename driver/src/main/java/org/neo4j/driver/internal/cluster/{ => loadbalancing}/LeastConnectedLoadBalancingStrategy.java (97%) rename driver/src/main/java/org/neo4j/driver/internal/cluster/{ => loadbalancing}/LoadBalancer.java (88%) rename driver/src/main/java/org/neo4j/driver/internal/cluster/{ => loadbalancing}/LoadBalancingStrategy.java (94%) rename driver/src/main/java/org/neo4j/driver/internal/cluster/{ => loadbalancing}/RoundRobinArrayIndex.java (96%) rename driver/src/main/java/org/neo4j/driver/internal/cluster/{ => loadbalancing}/RoundRobinLoadBalancingStrategy.java (96%) rename driver/src/test/java/org/neo4j/driver/internal/cluster/{ => loadbalancing}/LeastConnectedLoadBalancingStrategyTest.java (99%) rename driver/src/test/java/org/neo4j/driver/internal/cluster/{ => loadbalancing}/LoadBalancerTest.java (98%) rename driver/src/test/java/org/neo4j/driver/internal/cluster/{ => loadbalancing}/RoundRobinArrayIndexTest.java (97%) rename driver/src/test/java/org/neo4j/driver/internal/cluster/{ => loadbalancing}/RoundRobinLoadBalancingStrategyTest.java (98%) diff --git a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java index 6397e826ec..c1a4ead98d 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java +++ b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java @@ -22,9 +22,9 @@ import java.net.URI; import java.security.GeneralSecurityException; -import org.neo4j.driver.internal.cluster.LoadBalancer; import org.neo4j.driver.internal.cluster.RoutingContext; import org.neo4j.driver.internal.cluster.RoutingSettings; +import org.neo4j.driver.internal.cluster.loadbalancing.LoadBalancer; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.net.SocketConnector; import org.neo4j.driver.internal.net.pooling.PoolSettings; diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterComposition.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterComposition.java index 3a8ae556b4..7c9eceeaa7 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterComposition.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterComposition.java @@ -26,7 +26,7 @@ import org.neo4j.driver.v1.Value; import org.neo4j.driver.v1.util.Function; -final class ClusterComposition +public final class ClusterComposition { private static final long MAX_TTL = Long.MAX_VALUE / 1000L; private static final Function OF_BoltServerAddress = @@ -53,7 +53,7 @@ private ClusterComposition( long expirationTimestamp ) } /** For testing */ - ClusterComposition( + public ClusterComposition( long expirationTimestamp, Set readers, Set writers, diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoutingPooledConnection.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/RoutingPooledConnection.java index 7f7dd9e013..4c18466721 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoutingPooledConnection.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/RoutingPooledConnection.java @@ -35,13 +35,13 @@ import static java.lang.String.format; -class RoutingPooledConnection implements PooledConnection +public class RoutingPooledConnection implements PooledConnection { private final PooledConnection delegate; private final RoutingErrorHandler errorHandler; private final AccessMode accessMode; - RoutingPooledConnection( PooledConnection delegate, RoutingErrorHandler errorHandler, AccessMode accessMode ) + public RoutingPooledConnection( PooledConnection delegate, RoutingErrorHandler errorHandler, AccessMode accessMode ) { this.delegate = delegate; this.errorHandler = errorHandler; diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java similarity index 97% rename from driver/src/main/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategy.java rename to driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java index 030333b2ed..58367dd198 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategy.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java @@ -16,7 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.neo4j.driver.internal.cluster; +package org.neo4j.driver.internal.cluster.loadbalancing; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.spi.ConnectionPool; diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java similarity index 88% rename from driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java rename to driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java index 05c124303a..2e2f307788 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java @@ -16,11 +16,21 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.neo4j.driver.internal.cluster; +package org.neo4j.driver.internal.cluster.loadbalancing; import java.util.Set; import org.neo4j.driver.internal.RoutingErrorHandler; +import org.neo4j.driver.internal.cluster.AddressSet; +import org.neo4j.driver.internal.cluster.ClusterComposition; +import org.neo4j.driver.internal.cluster.ClusterCompositionProvider; +import org.neo4j.driver.internal.cluster.ClusterRoutingTable; +import org.neo4j.driver.internal.cluster.DnsResolver; +import org.neo4j.driver.internal.cluster.Rediscovery; +import org.neo4j.driver.internal.cluster.RoutingPooledConnection; +import org.neo4j.driver.internal.cluster.RoutingProcedureClusterCompositionProvider; +import org.neo4j.driver.internal.cluster.RoutingSettings; +import org.neo4j.driver.internal.cluster.RoutingTable; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.spi.ConnectionPool; import org.neo4j.driver.internal.spi.ConnectionProvider; @@ -55,7 +65,8 @@ private LoadBalancer( BoltServerAddress initialRouter, RoutingSettings settings, this( connections, routingTable, createRediscovery( initialRouter, settings, clock, log ), log ); } - LoadBalancer( ConnectionPool connections, RoutingTable routingTable, Rediscovery rediscovery, Logger log ) + // Used only in testing + public LoadBalancer( ConnectionPool connections, RoutingTable routingTable, Rediscovery rediscovery, Logger log ) { this.connections = connections; this.routingTable = routingTable; diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancingStrategy.java similarity index 94% rename from driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancingStrategy.java rename to driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancingStrategy.java index 99f40fcd42..e69e73bf30 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancingStrategy.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancingStrategy.java @@ -16,7 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.neo4j.driver.internal.cluster; +package org.neo4j.driver.internal.cluster.loadbalancing; import org.neo4j.driver.internal.net.BoltServerAddress; diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndex.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinArrayIndex.java similarity index 96% rename from driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndex.java rename to driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinArrayIndex.java index cc1002a875..19345f44f6 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndex.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinArrayIndex.java @@ -16,7 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.neo4j.driver.internal.cluster; +package org.neo4j.driver.internal.cluster.loadbalancing; import java.util.concurrent.atomic.AtomicInteger; diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategy.java similarity index 96% rename from driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategy.java rename to driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategy.java index 35cb127b85..b954cc3aa9 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategy.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategy.java @@ -16,7 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.neo4j.driver.internal.cluster; +package org.neo4j.driver.internal.cluster.loadbalancing; import org.neo4j.driver.internal.net.BoltServerAddress; diff --git a/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java b/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java index c4c8c5fd93..dd7be8391b 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java @@ -28,8 +28,8 @@ import java.util.Arrays; import java.util.List; -import org.neo4j.driver.internal.cluster.LoadBalancer; import org.neo4j.driver.internal.cluster.RoutingSettings; +import org.neo4j.driver.internal.cluster.loadbalancing.LoadBalancer; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.retry.RetryLogic; import org.neo4j.driver.internal.retry.RetrySettings; diff --git a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java index ec857b03e0..1abe36f727 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java @@ -29,8 +29,8 @@ import java.util.Collections; import java.util.Map; -import org.neo4j.driver.internal.cluster.LoadBalancer; import org.neo4j.driver.internal.cluster.RoutingSettings; +import org.neo4j.driver.internal.cluster.loadbalancing.LoadBalancer; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.retry.FixedRetryLogic; import org.neo4j.driver.internal.retry.RetryLogic; diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java index d559060bd7..ec842efc3e 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java @@ -30,6 +30,7 @@ import java.util.HashSet; import java.util.List; +import org.neo4j.driver.internal.cluster.loadbalancing.LoadBalancer; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.net.pooling.PoolSettings; import org.neo4j.driver.internal.net.pooling.SocketConnectionPool; diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategyTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategyTest.java similarity index 99% rename from driver/src/test/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategyTest.java rename to driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategyTest.java index 57b46d513d..91ac7ca4bd 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/LeastConnectedLoadBalancingStrategyTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategyTest.java @@ -16,7 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.neo4j.driver.internal.cluster; +package org.neo4j.driver.internal.cluster.loadbalancing; import org.junit.Before; import org.junit.Test; diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java similarity index 98% rename from driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java rename to driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java index 2b2134548a..cac64f12a9 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java @@ -16,7 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.neo4j.driver.internal.cluster; +package org.neo4j.driver.internal.cluster.loadbalancing; import org.junit.Test; import org.mockito.InOrder; @@ -32,6 +32,11 @@ import org.neo4j.driver.internal.ExplicitTransaction; import org.neo4j.driver.internal.NetworkSession; import org.neo4j.driver.internal.SessionResourcesHandler; +import org.neo4j.driver.internal.cluster.AddressSet; +import org.neo4j.driver.internal.cluster.ClusterComposition; +import org.neo4j.driver.internal.cluster.Rediscovery; +import org.neo4j.driver.internal.cluster.RoutingPooledConnection; +import org.neo4j.driver.internal.cluster.RoutingTable; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.retry.ExponentialBackoffRetryLogic; import org.neo4j.driver.internal.retry.RetryLogic; diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndexTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinArrayIndexTest.java similarity index 97% rename from driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndexTest.java rename to driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinArrayIndexTest.java index a91b678ca4..29171057cb 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinArrayIndexTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinArrayIndexTest.java @@ -16,7 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.neo4j.driver.internal.cluster; +package org.neo4j.driver.internal.cluster.loadbalancing; import org.junit.Test; diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategyTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategyTest.java similarity index 98% rename from driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategyTest.java rename to driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategyTest.java index 63ef0ad8e4..5df906da3e 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoundRobinLoadBalancingStrategyTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategyTest.java @@ -16,7 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.neo4j.driver.internal.cluster; +package org.neo4j.driver.internal.cluster.loadbalancing; import org.junit.Test; diff --git a/driver/src/test/java/org/neo4j/driver/internal/util/Matchers.java b/driver/src/test/java/org/neo4j/driver/internal/util/Matchers.java index a734dae5a1..05a0ac26a8 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/util/Matchers.java +++ b/driver/src/test/java/org/neo4j/driver/internal/util/Matchers.java @@ -29,8 +29,8 @@ import org.neo4j.driver.internal.SessionFactory; import org.neo4j.driver.internal.SessionFactoryImpl; import org.neo4j.driver.internal.cluster.AddressSet; -import org.neo4j.driver.internal.cluster.LoadBalancer; import org.neo4j.driver.internal.cluster.RoutingTable; +import org.neo4j.driver.internal.cluster.loadbalancing.LoadBalancer; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.spi.ConnectionProvider; import org.neo4j.driver.v1.Driver; From 5ea5cd18356aa34e7d5f28b50494e55def2f7fa9 Mon Sep 17 00:00:00 2001 From: lutovich Date: Wed, 5 Jul 2017 17:43:18 +0200 Subject: [PATCH 04/10] Experimental kill switch for using round robin load balancing --- .../neo4j/driver/internal/DriverFactory.java | 19 ++++++++- .../cluster/loadbalancing/LoadBalancer.java | 15 ++++--- .../main/java/org/neo4j/driver/v1/Config.java | 39 +++++++++++++++++++ .../driver/internal/RoutingDriverTest.java | 4 +- ...tingPooledConnectionErrorHandlingTest.java | 4 +- .../loadbalancing/LoadBalancerTest.java | 30 +++++++++----- 6 files changed, 92 insertions(+), 19 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java index c1a4ead98d..d2fa9d4c8b 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java +++ b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java @@ -24,7 +24,10 @@ import org.neo4j.driver.internal.cluster.RoutingContext; import org.neo4j.driver.internal.cluster.RoutingSettings; +import org.neo4j.driver.internal.cluster.loadbalancing.LeastConnectedLoadBalancingStrategy; import org.neo4j.driver.internal.cluster.loadbalancing.LoadBalancer; +import org.neo4j.driver.internal.cluster.loadbalancing.LoadBalancingStrategy; +import org.neo4j.driver.internal.cluster.loadbalancing.RoundRobinLoadBalancingStrategy; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.net.SocketConnector; import org.neo4j.driver.internal.net.pooling.PoolSettings; @@ -146,7 +149,21 @@ protected InternalDriver createDriver( Config config, SecurityPlan securityPlan, protected LoadBalancer createLoadBalancer( BoltServerAddress address, ConnectionPool connectionPool, Config config, RoutingSettings routingSettings ) { - return new LoadBalancer( address, routingSettings, connectionPool, createClock(), config.logging() ); + return new LoadBalancer( address, routingSettings, connectionPool, createClock(), config.logging(), + loadBalancingStrategy( config, connectionPool ) ); + } + + private LoadBalancingStrategy loadBalancingStrategy( Config config, + ConnectionPool connectionPool ) + { + if ( config.loadBalancingStrategy() == Config.LoadBalancingStrategy.ROUND_ROBIN ) + { + return new RoundRobinLoadBalancingStrategy(); + } + else + { + return new LeastConnectedLoadBalancingStrategy( connectionPool ); + } } /** diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java index 2e2f307788..fba909bf5c 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java @@ -53,25 +53,28 @@ public class LoadBalancer implements ConnectionProvider, RoutingErrorHandler, Au private final Logger log; public LoadBalancer( BoltServerAddress initialRouter, RoutingSettings settings, ConnectionPool connections, - Clock clock, Logging logging ) + Clock clock, Logging logging, LoadBalancingStrategy loadBalancingStrategy ) { this( initialRouter, settings, connections, new ClusterRoutingTable( clock, initialRouter ), clock, - logging.getLog( LOAD_BALANCER_LOG_NAME ) ); + logging.getLog( LOAD_BALANCER_LOG_NAME ), loadBalancingStrategy ); } private LoadBalancer( BoltServerAddress initialRouter, RoutingSettings settings, ConnectionPool connections, - RoutingTable routingTable, Clock clock, Logger log ) + RoutingTable routingTable, Clock clock, Logger log, + LoadBalancingStrategy loadBalancingStrategy ) { - this( connections, routingTable, createRediscovery( initialRouter, settings, clock, log ), log ); + this( connections, routingTable, createRediscovery( initialRouter, settings, clock, log ), log, + loadBalancingStrategy ); } // Used only in testing - public LoadBalancer( ConnectionPool connections, RoutingTable routingTable, Rediscovery rediscovery, Logger log ) + public LoadBalancer( ConnectionPool connections, RoutingTable routingTable, Rediscovery rediscovery, Logger log, + LoadBalancingStrategy loadBalancingStrategy ) { this.connections = connections; this.routingTable = routingTable; this.rediscovery = rediscovery; - this.loadBalancingStrategy = new LeastConnectedLoadBalancingStrategy( connections ); + this.loadBalancingStrategy = loadBalancingStrategy; this.log = log; refreshRoutingTable(); diff --git a/driver/src/main/java/org/neo4j/driver/v1/Config.java b/driver/src/main/java/org/neo4j/driver/v1/Config.java index 7e9c0ec5af..454c4d1585 100644 --- a/driver/src/main/java/org/neo4j/driver/v1/Config.java +++ b/driver/src/main/java/org/neo4j/driver/v1/Config.java @@ -29,6 +29,7 @@ import org.neo4j.driver.v1.exceptions.ServiceUnavailableException; import org.neo4j.driver.v1.exceptions.SessionExpiredException; import org.neo4j.driver.v1.exceptions.TransientException; +import org.neo4j.driver.v1.util.Experimental; import org.neo4j.driver.v1.util.Immutable; import org.neo4j.driver.v1.util.Resource; @@ -74,6 +75,8 @@ public class Config private final int connectionTimeoutMillis; private final RetrySettings retrySettings; + private final LoadBalancingStrategy loadBalancingStrategy; + private Config( ConfigBuilder builder) { this.logging = builder.logging; @@ -88,6 +91,17 @@ private Config( ConfigBuilder builder) this.routingRetryDelayMillis = builder.routingRetryDelayMillis; this.connectionTimeoutMillis = builder.connectionTimeoutMillis; this.retrySettings = builder.retrySettings; + this.loadBalancingStrategy = builder.loadBalancingStrategy; + } + + /** + * Load Balancing Strategy + * + * @return load balancing strategy to use + */ + public LoadBalancingStrategy loadBalancingStrategy() + { + return loadBalancingStrategy; } /** @@ -210,6 +224,7 @@ public static class ConfigBuilder private long idleTimeBeforeConnectionTest = PoolSettings.DEFAULT_IDLE_TIME_BEFORE_CONNECTION_TEST; private boolean encrypted = true; private TrustStrategy trustStrategy = trustAllCertificates(); + private LoadBalancingStrategy loadBalancingStrategy = LoadBalancingStrategy.LEAST_CONNECTED; private int routingFailureLimit = 1; private long routingRetryDelayMillis = TimeUnit.SECONDS.toMillis( 5 ); private int connectionTimeoutMillis = (int) TimeUnit.SECONDS.toMillis( 5 ); @@ -229,6 +244,23 @@ public ConfigBuilder withLogging( Logging logging ) return this; } + /** + * Provide an alternative load balancing implementation for the driver to use. By default we use + * {@link LoadBalancingStrategy#LEAST_CONNECTED}. + *

+ * We are experimnenting with different strategies before sticking to one. This could be removed in the next + * minor version + * + * @param loadBalancingStrategy the strategy to use + * @return this builder + */ + @Experimental + public ConfigBuilder withLoadBalancingStrategy( LoadBalancingStrategy loadBalancingStrategy ) + { + this.loadBalancingStrategy = loadBalancingStrategy; + return this; + } + /** * Enable logging of leaked sessions. *

@@ -542,6 +574,13 @@ public enum EncryptionLevel REQUIRED } + @Experimental + public enum LoadBalancingStrategy + { + ROUND_ROBIN, + LEAST_CONNECTED + } + /** * Control how the driver determines if it can trust the encryption certificates provided by the Neo4j instance it is connected to. */ diff --git a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java index 1abe36f727..0768b2180a 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java @@ -30,6 +30,7 @@ import java.util.Map; import org.neo4j.driver.internal.cluster.RoutingSettings; +import org.neo4j.driver.internal.cluster.loadbalancing.LeastConnectedLoadBalancingStrategy; import org.neo4j.driver.internal.cluster.loadbalancing.LoadBalancer; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.retry.FixedRetryLogic; @@ -347,7 +348,8 @@ private final Driver driverWithServers( long ttl, Map... serverIn private Driver driverWithPool( ConnectionPool pool ) { RoutingSettings settings = new RoutingSettings( 10, 5_000, null ); - ConnectionProvider connectionProvider = new LoadBalancer( SEED, settings, pool, clock, logging ); + ConnectionProvider connectionProvider = new LoadBalancer( SEED, settings, pool, clock, logging, + new LeastConnectedLoadBalancingStrategy( pool ) ); Config config = Config.build().withLogging( logging ).toConfig(); SessionFactory sessionFactory = new NetworkSessionWithAddressFactory( connectionProvider, config ); return new InternalDriver( insecure(), sessionFactory, logging ); diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java index ec842efc3e..d0f7038a62 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java @@ -30,6 +30,7 @@ import java.util.HashSet; import java.util.List; +import org.neo4j.driver.internal.cluster.loadbalancing.LeastConnectedLoadBalancingStrategy; import org.neo4j.driver.internal.cluster.loadbalancing.LoadBalancer; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.net.pooling.PoolSettings; @@ -325,7 +326,8 @@ private static LoadBalancer newLoadBalancer( ClusterComposition clusterCompositi { Rediscovery rediscovery = mock( Rediscovery.class ); when( rediscovery.lookupClusterComposition( routingTable, connectionPool ) ).thenReturn( clusterComposition ); - return new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + return new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER, + new LeastConnectedLoadBalancingStrategy( connectionPool ) ); } private interface ConnectionMethod diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java index cac64f12a9..26d1386a12 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java @@ -93,7 +93,8 @@ public void ensureRoutingShouldUpdateRoutingTableAndPurgeConnectionPoolWhenStale when( routingTable.update( any( ClusterComposition.class ) ) ).thenReturn( set ); // when - LoadBalancer balancer = new LoadBalancer( conns, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer balancer = new LoadBalancer( conns, routingTable, rediscovery, DEV_NULL_LOGGER, + new LeastConnectedLoadBalancingStrategy( conns ) ); // then assertNotNull( balancer ); @@ -109,7 +110,8 @@ public void shouldRefreshRoutingTableOnInitialization() throws Exception // given & when final AtomicInteger refreshRoutingTableCounter = new AtomicInteger( 0 ); LoadBalancer balancer = new LoadBalancer( mock( ConnectionPool.class ), mock( RoutingTable.class ), - mock( Rediscovery.class ), DEV_NULL_LOGGER ) + mock( Rediscovery.class ), DEV_NULL_LOGGER, + new LeastConnectedLoadBalancingStrategy( mock( ConnectionPool.class ) ) ) { @Override synchronized void refreshRoutingTable() @@ -163,7 +165,8 @@ public void shouldForgetAddressAndItsConnectionsOnServiceUnavailableWhileClosing RoutingTable routingTable = mock( RoutingTable.class ); ConnectionPool connectionPool = mock( ConnectionPool.class ); Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER, + new LeastConnectedLoadBalancingStrategy( connectionPool ) ); BoltServerAddress address = new BoltServerAddress( "host", 42 ); PooledConnection connection = newConnectionWithFailingSync( address ); @@ -197,7 +200,8 @@ public void shouldForgetAddressAndItsConnectionsOnServiceUnavailableWhileClosing PooledConnection connectionWithFailingSync = newConnectionWithFailingSync( address ); when( connectionPool.acquire( any( BoltServerAddress.class ) ) ).thenReturn( connectionWithFailingSync ); Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER, + new LeastConnectedLoadBalancingStrategy( connectionPool ) ); Session session = newSession( loadBalancer ); // begin transaction to make session obtain a connection @@ -243,7 +247,8 @@ public void shouldThrowWhenRediscoveryReturnsNoSuitableServers() when( routingTable.readers() ).thenReturn( new AddressSet() ); when( routingTable.writers() ).thenReturn( new AddressSet() ); - LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER, + new LeastConnectedLoadBalancingStrategy( connections ) ); try { @@ -283,7 +288,8 @@ public void shouldSelectLeastConnectedAddress() Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER, + new LeastConnectedLoadBalancingStrategy( connectionPool ) ); Set seenAddresses = new HashSet<>(); for ( int i = 0; i < 10; i++ ) @@ -309,7 +315,8 @@ public void shouldRoundRobinWhenNoActiveConnections() Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER, + new LeastConnectedLoadBalancingStrategy( connectionPool ) ); Set seenAddresses = new HashSet<>(); for ( int i = 0; i < 10; i++ ) @@ -330,7 +337,8 @@ private void testRediscoveryWhenStale( AccessMode mode ) RoutingTable routingTable = newStaleRoutingTableMock( mode ); Rediscovery rediscovery = newRediscoveryMock(); - LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER, + new LeastConnectedLoadBalancingStrategy( connections ) ); verify( rediscovery ).lookupClusterComposition( routingTable, connections ); assertNotNull( loadBalancer.acquireConnection( mode ) ); @@ -346,7 +354,8 @@ private void testNoRediscoveryWhenNotStale( AccessMode staleMode, AccessMode not RoutingTable routingTable = newStaleRoutingTableMock( staleMode ); Rediscovery rediscovery = newRediscoveryMock(); - LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER, + new LeastConnectedLoadBalancingStrategy( connections ) ); verify( rediscovery ).lookupClusterComposition( routingTable, connections ); assertNotNull( loadBalancer.acquireConnection( notStaleMode ) ); @@ -379,7 +388,8 @@ private LoadBalancer setupLoadBalancer( PooledConnection writerConn, PooledConne when( routingTable.readers() ).thenReturn( readerAddrs ); when( routingTable.writers() ).thenReturn( writerAddrs ); - return new LoadBalancer( connPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + return new LoadBalancer( connPool, routingTable, rediscovery, DEV_NULL_LOGGER, + new LeastConnectedLoadBalancingStrategy( connPool ) ); } private static Session newSession( LoadBalancer loadBalancer ) From f40118c7d7493348ee75f1722af2fe19d3628231 Mon Sep 17 00:00:00 2001 From: lutovich Date: Wed, 5 Jul 2017 18:59:36 +0200 Subject: [PATCH 05/10] Minor cleanups --- .../neo4j/driver/internal/DriverFactory.java | 14 +++--- .../LeastConnectedLoadBalancingStrategy.java | 48 +++++++++---------- .../cluster/loadbalancing/LoadBalancer.java | 24 ++++++---- .../main/java/org/neo4j/driver/v1/Config.java | 26 +++++----- ...tingPooledConnectionErrorHandlingTest.java | 4 +- .../loadbalancing/LoadBalancerTest.java | 30 ++++-------- 6 files changed, 68 insertions(+), 78 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java index d2fa9d4c8b..7f5186e140 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java +++ b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java @@ -150,19 +150,19 @@ protected LoadBalancer createLoadBalancer( BoltServerAddress address, Connection RoutingSettings routingSettings ) { return new LoadBalancer( address, routingSettings, connectionPool, createClock(), config.logging(), - loadBalancingStrategy( config, connectionPool ) ); + createLoadBalancingStrategy( config, connectionPool ) ); } - private LoadBalancingStrategy loadBalancingStrategy( Config config, - ConnectionPool connectionPool ) + private LoadBalancingStrategy createLoadBalancingStrategy( Config config, ConnectionPool connectionPool ) { - if ( config.loadBalancingStrategy() == Config.LoadBalancingStrategy.ROUND_ROBIN ) + switch ( config.loadBalancingStrategy() ) { + case ROUND_ROBIN: return new RoundRobinLoadBalancingStrategy(); - } - else - { + case LEAST_CONNECTED: return new LeastConnectedLoadBalancingStrategy( connectionPool ); + default: + throw new IllegalArgumentException( "Unknown load balancing strategy: " + config.loadBalancingStrategy() ); } } diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java index 58367dd198..b840b750a7 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java @@ -52,37 +52,35 @@ private BoltServerAddress select( BoltServerAddress[] addresses, RoundRobinArray { return null; } - else - { - int startIndex = addressesIndex.next( size ); - int index = startIndex; - BoltServerAddress leastConnectedAddress = null; - int leastActiveConnections = Integer.MAX_VALUE; + int startIndex = addressesIndex.next( size ); + int index = startIndex; - do - { - BoltServerAddress address = addresses[index]; - int activeConnections = connectionPool.activeConnections( address ); + BoltServerAddress leastConnectedAddress = null; + int leastActiveConnections = Integer.MAX_VALUE; - if ( activeConnections < leastActiveConnections ) - { - leastConnectedAddress = address; - leastActiveConnections = activeConnections; - } + do + { + BoltServerAddress address = addresses[index]; + int activeConnections = connectionPool.activeConnections( address ); - if ( index == size - 1 ) - { - index = 0; - } - else - { - index++; - } + if ( activeConnections < leastActiveConnections ) + { + leastConnectedAddress = address; + leastActiveConnections = activeConnections; } - while ( index != startIndex ); - return leastConnectedAddress; + if ( index == size - 1 ) + { + index = 0; + } + else + { + index++; + } } + while ( index != startIndex ); + + return leastConnectedAddress; } } diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java index fba909bf5c..70233c8f9d 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java @@ -55,20 +55,18 @@ public class LoadBalancer implements ConnectionProvider, RoutingErrorHandler, Au public LoadBalancer( BoltServerAddress initialRouter, RoutingSettings settings, ConnectionPool connections, Clock clock, Logging logging, LoadBalancingStrategy loadBalancingStrategy ) { - this( initialRouter, settings, connections, new ClusterRoutingTable( clock, initialRouter ), clock, - logging.getLog( LOAD_BALANCER_LOG_NAME ), loadBalancingStrategy ); + this( connections, new ClusterRoutingTable( clock, initialRouter ), + createRediscovery( initialRouter, settings, clock, logging ), loadBalancerLogger( logging ), + loadBalancingStrategy ); } - private LoadBalancer( BoltServerAddress initialRouter, RoutingSettings settings, ConnectionPool connections, - RoutingTable routingTable, Clock clock, Logger log, - LoadBalancingStrategy loadBalancingStrategy ) + // Used only in testing + public LoadBalancer( ConnectionPool connections, RoutingTable routingTable, Rediscovery rediscovery, Logger log ) { - this( connections, routingTable, createRediscovery( initialRouter, settings, clock, log ), log, - loadBalancingStrategy ); + this( connections, routingTable, rediscovery, log, new LeastConnectedLoadBalancingStrategy( connections ) ); } - // Used only in testing - public LoadBalancer( ConnectionPool connections, RoutingTable routingTable, Rediscovery rediscovery, Logger log, + private LoadBalancer( ConnectionPool connections, RoutingTable routingTable, Rediscovery rediscovery, Logger log, LoadBalancingStrategy loadBalancingStrategy ) { this.connections = connections; @@ -186,13 +184,19 @@ private BoltServerAddress selectAddress( AccessMode mode, AddressSet servers ) } private static Rediscovery createRediscovery( BoltServerAddress initialRouter, RoutingSettings settings, - Clock clock, Logger log ) + Clock clock, Logging logging ) { + Logger log = loadBalancerLogger( logging ); ClusterCompositionProvider clusterComposition = new RoutingProcedureClusterCompositionProvider( clock, log, settings ); return new Rediscovery( initialRouter, settings, clock, log, clusterComposition, new DnsResolver( log ) ); } + private static Logger loadBalancerLogger( Logging logging ) + { + return logging.getLog( LOAD_BALANCER_LOG_NAME ); + } + private static RuntimeException unknownMode( AccessMode mode ) { return new IllegalArgumentException( "Mode '" + mode + "' is not supported" ); diff --git a/driver/src/main/java/org/neo4j/driver/v1/Config.java b/driver/src/main/java/org/neo4j/driver/v1/Config.java index 454c4d1585..2d10181705 100644 --- a/driver/src/main/java/org/neo4j/driver/v1/Config.java +++ b/driver/src/main/java/org/neo4j/driver/v1/Config.java @@ -94,16 +94,6 @@ private Config( ConfigBuilder builder) this.loadBalancingStrategy = builder.loadBalancingStrategy; } - /** - * Load Balancing Strategy - * - * @return load balancing strategy to use - */ - public LoadBalancingStrategy loadBalancingStrategy() - { - return loadBalancingStrategy; - } - /** * Logging provider * @return the logging provider to use @@ -186,6 +176,17 @@ public TrustStrategy trustStrategy() return trustStrategy; } + /** + * Load balancing strategy + * + * @return the strategy to use + */ + @Experimental + public LoadBalancingStrategy loadBalancingStrategy() + { + return loadBalancingStrategy; + } + /** * Return a {@link ConfigBuilder} instance * @return a {@link ConfigBuilder} instance @@ -245,11 +246,10 @@ public ConfigBuilder withLogging( Logging logging ) } /** - * Provide an alternative load balancing implementation for the driver to use. By default we use + * Provide an alternative load balancing strategy for the routing driver to use. By default we use * {@link LoadBalancingStrategy#LEAST_CONNECTED}. *

- * We are experimnenting with different strategies before sticking to one. This could be removed in the next - * minor version + * Note: We are experimenting with different strategies. This could be removed in the next minor version. * * @param loadBalancingStrategy the strategy to use * @return this builder diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java index d0f7038a62..ec842efc3e 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java @@ -30,7 +30,6 @@ import java.util.HashSet; import java.util.List; -import org.neo4j.driver.internal.cluster.loadbalancing.LeastConnectedLoadBalancingStrategy; import org.neo4j.driver.internal.cluster.loadbalancing.LoadBalancer; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.net.pooling.PoolSettings; @@ -326,8 +325,7 @@ private static LoadBalancer newLoadBalancer( ClusterComposition clusterCompositi { Rediscovery rediscovery = mock( Rediscovery.class ); when( rediscovery.lookupClusterComposition( routingTable, connectionPool ) ).thenReturn( clusterComposition ); - return new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER, - new LeastConnectedLoadBalancingStrategy( connectionPool ) ); + return new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); } private interface ConnectionMethod diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java index 26d1386a12..cac64f12a9 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java @@ -93,8 +93,7 @@ public void ensureRoutingShouldUpdateRoutingTableAndPurgeConnectionPoolWhenStale when( routingTable.update( any( ClusterComposition.class ) ) ).thenReturn( set ); // when - LoadBalancer balancer = new LoadBalancer( conns, routingTable, rediscovery, DEV_NULL_LOGGER, - new LeastConnectedLoadBalancingStrategy( conns ) ); + LoadBalancer balancer = new LoadBalancer( conns, routingTable, rediscovery, DEV_NULL_LOGGER ); // then assertNotNull( balancer ); @@ -110,8 +109,7 @@ public void shouldRefreshRoutingTableOnInitialization() throws Exception // given & when final AtomicInteger refreshRoutingTableCounter = new AtomicInteger( 0 ); LoadBalancer balancer = new LoadBalancer( mock( ConnectionPool.class ), mock( RoutingTable.class ), - mock( Rediscovery.class ), DEV_NULL_LOGGER, - new LeastConnectedLoadBalancingStrategy( mock( ConnectionPool.class ) ) ) + mock( Rediscovery.class ), DEV_NULL_LOGGER ) { @Override synchronized void refreshRoutingTable() @@ -165,8 +163,7 @@ public void shouldForgetAddressAndItsConnectionsOnServiceUnavailableWhileClosing RoutingTable routingTable = mock( RoutingTable.class ); ConnectionPool connectionPool = mock( ConnectionPool.class ); Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER, - new LeastConnectedLoadBalancingStrategy( connectionPool ) ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); BoltServerAddress address = new BoltServerAddress( "host", 42 ); PooledConnection connection = newConnectionWithFailingSync( address ); @@ -200,8 +197,7 @@ public void shouldForgetAddressAndItsConnectionsOnServiceUnavailableWhileClosing PooledConnection connectionWithFailingSync = newConnectionWithFailingSync( address ); when( connectionPool.acquire( any( BoltServerAddress.class ) ) ).thenReturn( connectionWithFailingSync ); Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER, - new LeastConnectedLoadBalancingStrategy( connectionPool ) ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); Session session = newSession( loadBalancer ); // begin transaction to make session obtain a connection @@ -247,8 +243,7 @@ public void shouldThrowWhenRediscoveryReturnsNoSuitableServers() when( routingTable.readers() ).thenReturn( new AddressSet() ); when( routingTable.writers() ).thenReturn( new AddressSet() ); - LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER, - new LeastConnectedLoadBalancingStrategy( connections ) ); + LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER ); try { @@ -288,8 +283,7 @@ public void shouldSelectLeastConnectedAddress() Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER, - new LeastConnectedLoadBalancingStrategy( connectionPool ) ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); Set seenAddresses = new HashSet<>(); for ( int i = 0; i < 10; i++ ) @@ -315,8 +309,7 @@ public void shouldRoundRobinWhenNoActiveConnections() Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER, - new LeastConnectedLoadBalancingStrategy( connectionPool ) ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); Set seenAddresses = new HashSet<>(); for ( int i = 0; i < 10; i++ ) @@ -337,8 +330,7 @@ private void testRediscoveryWhenStale( AccessMode mode ) RoutingTable routingTable = newStaleRoutingTableMock( mode ); Rediscovery rediscovery = newRediscoveryMock(); - LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER, - new LeastConnectedLoadBalancingStrategy( connections ) ); + LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER ); verify( rediscovery ).lookupClusterComposition( routingTable, connections ); assertNotNull( loadBalancer.acquireConnection( mode ) ); @@ -354,8 +346,7 @@ private void testNoRediscoveryWhenNotStale( AccessMode staleMode, AccessMode not RoutingTable routingTable = newStaleRoutingTableMock( staleMode ); Rediscovery rediscovery = newRediscoveryMock(); - LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER, - new LeastConnectedLoadBalancingStrategy( connections ) ); + LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER ); verify( rediscovery ).lookupClusterComposition( routingTable, connections ); assertNotNull( loadBalancer.acquireConnection( notStaleMode ) ); @@ -388,8 +379,7 @@ private LoadBalancer setupLoadBalancer( PooledConnection writerConn, PooledConne when( routingTable.readers() ).thenReturn( readerAddrs ); when( routingTable.writers() ).thenReturn( writerAddrs ); - return new LoadBalancer( connPool, routingTable, rediscovery, DEV_NULL_LOGGER, - new LeastConnectedLoadBalancingStrategy( connPool ) ); + return new LoadBalancer( connPool, routingTable, rediscovery, DEV_NULL_LOGGER ); } private static Session newSession( LoadBalancer loadBalancer ) From 798b4a0cb7fbec1f6f5423d9c5a3c334eb855d40 Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 6 Jul 2017 10:03:26 +0200 Subject: [PATCH 06/10] Added some javadocs --- .../LeastConnectedLoadBalancingStrategy.java | 8 ++++++++ .../loadbalancing/LoadBalancingStrategy.java | 15 +++++++++++++++ .../RoundRobinLoadBalancingStrategy.java | 4 ++++ 3 files changed, 27 insertions(+) diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java index b840b750a7..38e4f0c29e 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java @@ -21,6 +21,11 @@ import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.spi.ConnectionPool; +/** + * Load balancing strategy that finds server with least amount of active (checked out of the pool) connections from + * given readers or writers. It finds a start index for iteration in a round-robin fashion. This is done to prevent + * choosing same first address over and over when all addresses have same amount of active connections. + */ public class LeastConnectedLoadBalancingStrategy implements LoadBalancingStrategy { private final RoundRobinArrayIndex readersIndex = new RoundRobinArrayIndex(); @@ -53,12 +58,14 @@ private BoltServerAddress select( BoltServerAddress[] addresses, RoundRobinArray return null; } + // choose start index for iteration in round-rodin fashion int startIndex = addressesIndex.next( size ); int index = startIndex; BoltServerAddress leastConnectedAddress = null; int leastActiveConnections = Integer.MAX_VALUE; + // iterate over the array to find least connected address do { BoltServerAddress address = addresses[index]; @@ -70,6 +77,7 @@ private BoltServerAddress select( BoltServerAddress[] addresses, RoundRobinArray leastActiveConnections = activeConnections; } + // loop over to the start of the array when end is reached if ( index == size - 1 ) { index = 0; diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancingStrategy.java index e69e73bf30..0b3ee3c8e1 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancingStrategy.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancingStrategy.java @@ -20,9 +20,24 @@ import org.neo4j.driver.internal.net.BoltServerAddress; +/** + * A facility to select most appropriate reader or writer among the given addresses for request processing. + */ public interface LoadBalancingStrategy { + /** + * Select most appropriate read address from the given array of addresses. + * + * @param knownReaders array of all known readers. + * @return most appropriate reader or {@code null} if it can't be selected. + */ BoltServerAddress selectReader( BoltServerAddress[] knownReaders ); + /** + * Select most appropriate write address from the given array of addresses. + * + * @param knownWriters array of all known writers. + * @return most appropriate writer or {@code null} if it can't be selected. + */ BoltServerAddress selectWriter( BoltServerAddress[] knownWriters ); } diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategy.java index b954cc3aa9..5d5894b938 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategy.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategy.java @@ -20,6 +20,10 @@ import org.neo4j.driver.internal.net.BoltServerAddress; +/** + * Load balancing strategy that selects addresses in round-robin fashion. It maintains separate indices for readers and + * writers. + */ public class RoundRobinLoadBalancingStrategy implements LoadBalancingStrategy { private final RoundRobinArrayIndex readersIndex = new RoundRobinArrayIndex(); From 1240e48537e486c6a5c3ed523bb20aad23de0cc2 Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 6 Jul 2017 14:00:08 +0200 Subject: [PATCH 07/10] Added trace logging to load balancing strategies --- .../neo4j/driver/internal/DriverFactory.java | 4 +- .../LeastConnectedLoadBalancingStrategy.java | 19 ++++++-- .../cluster/loadbalancing/LoadBalancer.java | 6 ++- .../RoundRobinLoadBalancingStrategy.java | 24 ++++++++-- .../driver/internal/RoutingDriverTest.java | 2 +- ...tingPooledConnectionErrorHandlingTest.java | 3 +- ...astConnectedLoadBalancingStrategyTest.java | 46 ++++++++++++++++++- .../loadbalancing/LoadBalancerTest.java | 21 ++++----- .../RoundRobinLoadBalancingStrategyTest.java | 44 +++++++++++++++++- 9 files changed, 141 insertions(+), 28 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java index 7f5186e140..b741e82c07 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java +++ b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java @@ -158,9 +158,9 @@ private LoadBalancingStrategy createLoadBalancingStrategy( Config config, Connec switch ( config.loadBalancingStrategy() ) { case ROUND_ROBIN: - return new RoundRobinLoadBalancingStrategy(); + return new RoundRobinLoadBalancingStrategy( config.logging() ); case LEAST_CONNECTED: - return new LeastConnectedLoadBalancingStrategy( connectionPool ); + return new LeastConnectedLoadBalancingStrategy( connectionPool, config.logging() ); default: throw new IllegalArgumentException( "Unknown load balancing strategy: " + config.loadBalancingStrategy() ); } diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java index 38e4f0c29e..d9157d6f21 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategy.java @@ -20,6 +20,8 @@ import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.spi.ConnectionPool; +import org.neo4j.driver.v1.Logger; +import org.neo4j.driver.v1.Logging; /** * Load balancing strategy that finds server with least amount of active (checked out of the pool) connections from @@ -28,33 +30,39 @@ */ public class LeastConnectedLoadBalancingStrategy implements LoadBalancingStrategy { + private static final String LOGGER_NAME = LeastConnectedLoadBalancingStrategy.class.getSimpleName(); + private final RoundRobinArrayIndex readersIndex = new RoundRobinArrayIndex(); private final RoundRobinArrayIndex writersIndex = new RoundRobinArrayIndex(); private final ConnectionPool connectionPool; + private final Logger log; - public LeastConnectedLoadBalancingStrategy( ConnectionPool connectionPool ) + public LeastConnectedLoadBalancingStrategy( ConnectionPool connectionPool, Logging logging ) { this.connectionPool = connectionPool; + this.log = logging.getLog( LOGGER_NAME ); } @Override public BoltServerAddress selectReader( BoltServerAddress[] knownReaders ) { - return select( knownReaders, readersIndex ); + return select( knownReaders, readersIndex, "reader" ); } @Override public BoltServerAddress selectWriter( BoltServerAddress[] knownWriters ) { - return select( knownWriters, writersIndex ); + return select( knownWriters, writersIndex, "writer" ); } - private BoltServerAddress select( BoltServerAddress[] addresses, RoundRobinArrayIndex addressesIndex ) + private BoltServerAddress select( BoltServerAddress[] addresses, RoundRobinArrayIndex addressesIndex, + String addressType ) { int size = addresses.length; if ( size == 0 ) { + log.trace( "Unable to select %s, no known addresses given", addressType ); return null; } @@ -89,6 +97,9 @@ private BoltServerAddress select( BoltServerAddress[] addresses, RoundRobinArray } while ( index != startIndex ); + log.trace( "Selected %s with address: '%s' and active connections: %s", + addressType, leastConnectedAddress, leastActiveConnections ); + return leastConnectedAddress; } } diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java index 70233c8f9d..cc90958e0c 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancer.java @@ -61,9 +61,11 @@ public LoadBalancer( BoltServerAddress initialRouter, RoutingSettings settings, } // Used only in testing - public LoadBalancer( ConnectionPool connections, RoutingTable routingTable, Rediscovery rediscovery, Logger log ) + public LoadBalancer( ConnectionPool connections, RoutingTable routingTable, Rediscovery rediscovery, + Logging logging ) { - this( connections, routingTable, rediscovery, log, new LeastConnectedLoadBalancingStrategy( connections ) ); + this( connections, routingTable, rediscovery, loadBalancerLogger( logging ), + new LeastConnectedLoadBalancingStrategy( connections, logging ) ); } private LoadBalancer( ConnectionPool connections, RoutingTable routingTable, Rediscovery rediscovery, Logger log, diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategy.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategy.java index 5d5894b938..1a977cd65b 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategy.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategy.java @@ -19,6 +19,8 @@ package org.neo4j.driver.internal.cluster.loadbalancing; import org.neo4j.driver.internal.net.BoltServerAddress; +import org.neo4j.driver.v1.Logger; +import org.neo4j.driver.v1.Logging; /** * Load balancing strategy that selects addresses in round-robin fashion. It maintains separate indices for readers and @@ -26,29 +28,43 @@ */ public class RoundRobinLoadBalancingStrategy implements LoadBalancingStrategy { + private static final String LOGGER_NAME = RoundRobinLoadBalancingStrategy.class.getSimpleName(); + private final RoundRobinArrayIndex readersIndex = new RoundRobinArrayIndex(); private final RoundRobinArrayIndex writersIndex = new RoundRobinArrayIndex(); + private final Logger log; + + public RoundRobinLoadBalancingStrategy( Logging logging ) + { + this.log = logging.getLog( LOGGER_NAME ); + } + @Override public BoltServerAddress selectReader( BoltServerAddress[] knownReaders ) { - return select( knownReaders, readersIndex ); + return select( knownReaders, readersIndex, "reader" ); } @Override public BoltServerAddress selectWriter( BoltServerAddress[] knownWriters ) { - return select( knownWriters, writersIndex ); + return select( knownWriters, writersIndex, "writer" ); } - private BoltServerAddress select( BoltServerAddress[] addresses, RoundRobinArrayIndex roundRobinIndex ) + private BoltServerAddress select( BoltServerAddress[] addresses, RoundRobinArrayIndex roundRobinIndex, + String addressType ) { int length = addresses.length; if ( length == 0 ) { + log.trace( "Unable to select %s, no known addresses given", addressType ); return null; } + int index = roundRobinIndex.next( length ); - return addresses[index]; + BoltServerAddress address = addresses[index]; + log.trace( "Selected %s with address: '%s'", addressType, address ); + return address; } } diff --git a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java index 0768b2180a..9f7057c792 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java @@ -349,7 +349,7 @@ private Driver driverWithPool( ConnectionPool pool ) { RoutingSettings settings = new RoutingSettings( 10, 5_000, null ); ConnectionProvider connectionProvider = new LoadBalancer( SEED, settings, pool, clock, logging, - new LeastConnectedLoadBalancingStrategy( pool ) ); + new LeastConnectedLoadBalancingStrategy( pool, logging ) ); Config config = Config.build().withLogging( logging ).toConfig(); SessionFactory sessionFactory = new NetworkSessionWithAddressFactory( connectionProvider, config ); return new InternalDriver( insecure(), sessionFactory, logging ); diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java index ec842efc3e..34afd56386 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/RoutingPooledConnectionErrorHandlingTest.java @@ -55,7 +55,6 @@ import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -import static org.neo4j.driver.internal.logging.DevNullLogger.DEV_NULL_LOGGER; import static org.neo4j.driver.internal.logging.DevNullLogging.DEV_NULL_LOGGING; import static org.neo4j.driver.internal.net.pooling.PoolSettings.DEFAULT_MAX_IDLE_CONNECTION_POOL_SIZE; import static org.neo4j.driver.internal.net.pooling.PoolSettings.NO_IDLE_CONNECTION_TEST; @@ -325,7 +324,7 @@ private static LoadBalancer newLoadBalancer( ClusterComposition clusterCompositi { Rediscovery rediscovery = mock( Rediscovery.class ); when( rediscovery.lookupClusterComposition( routingTable, connectionPool ) ).thenReturn( clusterComposition ); - return new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + return new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGING ); } private interface ConnectionMethod diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategyTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategyTest.java index 91ac7ca4bd..722efc0bbd 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategyTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LeastConnectedLoadBalancingStrategyTest.java @@ -24,11 +24,21 @@ import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.spi.ConnectionPool; +import org.neo4j.driver.v1.Logger; +import org.neo4j.driver.v1.Logging; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.eq; +import static org.mockito.Matchers.startsWith; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.mockito.MockitoAnnotations.initMocks; +import static org.neo4j.driver.internal.cluster.ClusterCompositionUtil.A; +import static org.neo4j.driver.internal.logging.DevNullLogging.DEV_NULL_LOGGING; public class LeastConnectedLoadBalancingStrategyTest { @@ -40,7 +50,7 @@ public class LeastConnectedLoadBalancingStrategyTest public void setUp() throws Exception { initMocks( this ); - strategy = new LeastConnectedLoadBalancingStrategy( connectionPool ); + strategy = new LeastConnectedLoadBalancingStrategy( connectionPool, DEV_NULL_LOGGING ); } @Test @@ -148,4 +158,38 @@ public void shouldReturnDifferentWriterOnEveryInvocationWhenNoActiveConnections( assertEquals( address1, strategy.selectReader( new BoltServerAddress[]{address1, address2} ) ); assertEquals( address2, strategy.selectReader( new BoltServerAddress[]{address1, address2} ) ); } + + @Test + public void shouldTraceLogWhenNoAddressSelected() + { + Logging logging = mock( Logging.class ); + Logger logger = mock( Logger.class ); + when( logging.getLog( anyString() ) ).thenReturn( logger ); + + LoadBalancingStrategy strategy = new LeastConnectedLoadBalancingStrategy( connectionPool, logging ); + + strategy.selectReader( new BoltServerAddress[0] ); + strategy.selectWriter( new BoltServerAddress[0] ); + + verify( logger ).trace( startsWith( "Unable to select" ), eq( "reader" ) ); + verify( logger ).trace( startsWith( "Unable to select" ), eq( "writer" ) ); + } + + @Test + public void shouldTraceLogSelectedAddress() + { + Logging logging = mock( Logging.class ); + Logger logger = mock( Logger.class ); + when( logging.getLog( anyString() ) ).thenReturn( logger ); + + when( connectionPool.activeConnections( any( BoltServerAddress.class ) ) ).thenReturn( 42 ); + + LoadBalancingStrategy strategy = new LeastConnectedLoadBalancingStrategy( connectionPool, logging ); + + strategy.selectReader( new BoltServerAddress[]{A} ); + strategy.selectWriter( new BoltServerAddress[]{A} ); + + verify( logger ).trace( startsWith( "Selected" ), eq( "reader" ), eq( A ), eq( 42 ) ); + verify( logger ).trace( startsWith( "Selected" ), eq( "writer" ), eq( A ), eq( 42 ) ); + } } diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java index cac64f12a9..8cec050acc 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/LoadBalancerTest.java @@ -74,7 +74,6 @@ import static org.neo4j.driver.internal.cluster.ClusterCompositionUtil.A; import static org.neo4j.driver.internal.cluster.ClusterCompositionUtil.B; import static org.neo4j.driver.internal.cluster.ClusterCompositionUtil.C; -import static org.neo4j.driver.internal.logging.DevNullLogger.DEV_NULL_LOGGER; import static org.neo4j.driver.internal.logging.DevNullLogging.DEV_NULL_LOGGING; import static org.neo4j.driver.internal.net.BoltServerAddress.LOCAL_DEFAULT; import static org.neo4j.driver.v1.AccessMode.READ; @@ -93,7 +92,7 @@ public void ensureRoutingShouldUpdateRoutingTableAndPurgeConnectionPoolWhenStale when( routingTable.update( any( ClusterComposition.class ) ) ).thenReturn( set ); // when - LoadBalancer balancer = new LoadBalancer( conns, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer balancer = new LoadBalancer( conns, routingTable, rediscovery, DEV_NULL_LOGGING ); // then assertNotNull( balancer ); @@ -109,7 +108,7 @@ public void shouldRefreshRoutingTableOnInitialization() throws Exception // given & when final AtomicInteger refreshRoutingTableCounter = new AtomicInteger( 0 ); LoadBalancer balancer = new LoadBalancer( mock( ConnectionPool.class ), mock( RoutingTable.class ), - mock( Rediscovery.class ), DEV_NULL_LOGGER ) + mock( Rediscovery.class ), DEV_NULL_LOGGING ) { @Override synchronized void refreshRoutingTable() @@ -163,7 +162,7 @@ public void shouldForgetAddressAndItsConnectionsOnServiceUnavailableWhileClosing RoutingTable routingTable = mock( RoutingTable.class ); ConnectionPool connectionPool = mock( ConnectionPool.class ); Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGING ); BoltServerAddress address = new BoltServerAddress( "host", 42 ); PooledConnection connection = newConnectionWithFailingSync( address ); @@ -197,7 +196,7 @@ public void shouldForgetAddressAndItsConnectionsOnServiceUnavailableWhileClosing PooledConnection connectionWithFailingSync = newConnectionWithFailingSync( address ); when( connectionPool.acquire( any( BoltServerAddress.class ) ) ).thenReturn( connectionWithFailingSync ); Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGING ); Session session = newSession( loadBalancer ); // begin transaction to make session obtain a connection @@ -243,7 +242,7 @@ public void shouldThrowWhenRediscoveryReturnsNoSuitableServers() when( routingTable.readers() ).thenReturn( new AddressSet() ); when( routingTable.writers() ).thenReturn( new AddressSet() ); - LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGING ); try { @@ -283,7 +282,7 @@ public void shouldSelectLeastConnectedAddress() Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGING ); Set seenAddresses = new HashSet<>(); for ( int i = 0; i < 10; i++ ) @@ -309,7 +308,7 @@ public void shouldRoundRobinWhenNoActiveConnections() Rediscovery rediscovery = mock( Rediscovery.class ); - LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connectionPool, routingTable, rediscovery, DEV_NULL_LOGGING ); Set seenAddresses = new HashSet<>(); for ( int i = 0; i < 10; i++ ) @@ -330,7 +329,7 @@ private void testRediscoveryWhenStale( AccessMode mode ) RoutingTable routingTable = newStaleRoutingTableMock( mode ); Rediscovery rediscovery = newRediscoveryMock(); - LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGING ); verify( rediscovery ).lookupClusterComposition( routingTable, connections ); assertNotNull( loadBalancer.acquireConnection( mode ) ); @@ -346,7 +345,7 @@ private void testNoRediscoveryWhenNotStale( AccessMode staleMode, AccessMode not RoutingTable routingTable = newStaleRoutingTableMock( staleMode ); Rediscovery rediscovery = newRediscoveryMock(); - LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGER ); + LoadBalancer loadBalancer = new LoadBalancer( connections, routingTable, rediscovery, DEV_NULL_LOGGING ); verify( rediscovery ).lookupClusterComposition( routingTable, connections ); assertNotNull( loadBalancer.acquireConnection( notStaleMode ) ); @@ -379,7 +378,7 @@ private LoadBalancer setupLoadBalancer( PooledConnection writerConn, PooledConne when( routingTable.readers() ).thenReturn( readerAddrs ); when( routingTable.writers() ).thenReturn( writerAddrs ); - return new LoadBalancer( connPool, routingTable, rediscovery, DEV_NULL_LOGGER ); + return new LoadBalancer( connPool, routingTable, rediscovery, DEV_NULL_LOGGING ); } private static Session newSession( LoadBalancer loadBalancer ) diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategyTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategyTest.java index 5df906da3e..c1f016c411 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategyTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/loadbalancing/RoundRobinLoadBalancingStrategyTest.java @@ -21,13 +21,23 @@ import org.junit.Test; import org.neo4j.driver.internal.net.BoltServerAddress; +import org.neo4j.driver.v1.Logger; +import org.neo4j.driver.v1.Logging; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.eq; +import static org.mockito.Matchers.startsWith; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.neo4j.driver.internal.cluster.ClusterCompositionUtil.A; +import static org.neo4j.driver.internal.logging.DevNullLogging.DEV_NULL_LOGGING; public class RoundRobinLoadBalancingStrategyTest { - private final RoundRobinLoadBalancingStrategy strategy = new RoundRobinLoadBalancingStrategy(); + private final RoundRobinLoadBalancingStrategy strategy = new RoundRobinLoadBalancingStrategy( DEV_NULL_LOGGING ); @Test public void shouldHandleEmptyReadersArray() @@ -95,4 +105,36 @@ public void shouldReturnWriterInRoundRobinOrder() assertEquals( address2, strategy.selectWriter( writers ) ); assertEquals( address3, strategy.selectWriter( writers ) ); } + + @Test + public void shouldTraceLogWhenNoAddressSelected() + { + Logging logging = mock( Logging.class ); + Logger logger = mock( Logger.class ); + when( logging.getLog( anyString() ) ).thenReturn( logger ); + + RoundRobinLoadBalancingStrategy strategy = new RoundRobinLoadBalancingStrategy( logging ); + + strategy.selectReader( new BoltServerAddress[0] ); + strategy.selectWriter( new BoltServerAddress[0] ); + + verify( logger ).trace( startsWith( "Unable to select" ), eq( "reader" ) ); + verify( logger ).trace( startsWith( "Unable to select" ), eq( "writer" ) ); + } + + @Test + public void shouldTraceLogSelectedAddress() + { + Logging logging = mock( Logging.class ); + Logger logger = mock( Logger.class ); + when( logging.getLog( anyString() ) ).thenReturn( logger ); + + RoundRobinLoadBalancingStrategy strategy = new RoundRobinLoadBalancingStrategy( logging ); + + strategy.selectReader( new BoltServerAddress[]{A} ); + strategy.selectWriter( new BoltServerAddress[]{A} ); + + verify( logger ).trace( startsWith( "Selected" ), eq( "reader" ), eq( A ) ); + verify( logger ).trace( startsWith( "Selected" ), eq( "writer" ), eq( A ) ); + } } From d1ebf473f16ac593da7ce09a0551b852ddbfbf7c Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 6 Jul 2017 14:48:05 +0200 Subject: [PATCH 08/10] Added trace logging around connection pool --- .../BlockingPooledConnectionQueue.java | 34 ++++++++++++------- .../net/pooling/SocketConnectionPool.java | 5 +++ 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java b/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java index 89b90ad979..a17dc2a4c8 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java +++ b/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java @@ -18,9 +18,7 @@ */ package org.neo4j.driver.internal.net.pooling; -import java.util.ArrayList; import java.util.Collections; -import java.util.List; import java.util.Set; import java.util.concurrent.BlockingQueue; import java.util.concurrent.ConcurrentHashMap; @@ -40,8 +38,6 @@ */ public class BlockingPooledConnectionQueue { - public static final String LOG_NAME = "ConnectionQueue"; - /** The backing queue, keeps track of connections currently in queue */ private final BlockingQueue queue; private final Logger logger; @@ -69,7 +65,9 @@ public boolean offer( PooledConnection pooledConnection ) acquiredConnections.remove( pooledConnection ); boolean offer = queue.offer( pooledConnection ); // not added back to the queue, dispose of the connection - if (!offer) { + if ( !offer ) + { + trace( "Queue is at capacity. Offered connection will be disposed." ); pooledConnection.dispose(); } if (isTerminating.get()) { @@ -89,12 +87,16 @@ public boolean offer( PooledConnection pooledConnection ) */ public PooledConnection acquire( Supplier supplier ) { - PooledConnection connection = queue.poll(); if ( connection == null ) { + trace( "No idle connections. Creating new connection." ); connection = supplier.get(); } + else + { + trace( "Acquired an idle connection." ); + } acquiredConnections.add( connection ); if (isTerminating.get()) { @@ -105,11 +107,6 @@ public PooledConnection acquire( Supplier supplier ) return connection; } - public List toList() - { - return new ArrayList<>( queue ); - } - public boolean isEmpty() { return queue.isEmpty(); @@ -140,6 +137,8 @@ public void terminate() { if ( isTerminating.compareAndSet( false, true ) ) { + trace( "Initiating connection queue termination." ); + while ( !queue.isEmpty() ) { PooledConnection idleConnection = queue.poll(); @@ -170,6 +169,17 @@ private void disposeSafely( PooledConnection connection ) private static Logger createLogger( BoltServerAddress address, Logging logging ) { - return new DelegatingLogger( logging.getLog( LOG_NAME ), address.toString() ); + Logger log = logging.getLog( BlockingPooledConnectionQueue.class.getSimpleName() ); + return new DelegatingLogger( log, address.toString() ); + } + + private void trace( String message ) + { + // Call to activeConnections is costly. This if block is to avoid that. + if ( logger.isTraceEnabled() ) + { + logger.trace( "%s ActiveConnections %s IdleConnections %s", + message, activeConnections(), queue.size() ); + } } } diff --git a/driver/src/main/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPool.java b/driver/src/main/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPool.java index 9449a6f402..ef3b371368 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPool.java +++ b/driver/src/main/java/org/neo4j/driver/internal/net/pooling/SocketConnectionPool.java @@ -30,6 +30,7 @@ import org.neo4j.driver.internal.spi.PooledConnection; import org.neo4j.driver.internal.util.Clock; import org.neo4j.driver.internal.util.Supplier; +import org.neo4j.driver.v1.Logger; import org.neo4j.driver.v1.Logging; /** @@ -58,6 +59,7 @@ public class SocketConnectionPool implements ConnectionPool private final ConnectionValidator connectionValidator; private final Clock clock; private final Logging logging; + private final Logger logger; public SocketConnectionPool( PoolSettings poolSettings, Connector connector, Clock clock, Logging logging ) { @@ -66,6 +68,7 @@ public SocketConnectionPool( PoolSettings poolSettings, Connector connector, Clo this.connectionValidator = new PooledConnectionValidator( this ); this.clock = clock; this.logging = logging; + this.logger = logging.getLog( SocketConnectionPool.class.getSimpleName() ); } @Override @@ -85,6 +88,7 @@ public void purge( BoltServerAddress address ) BlockingPooledConnectionQueue connections = pools.remove( address ); if ( connections != null ) { + logger.trace( "Purging pool for address %s", address ); connections.terminate(); } } @@ -107,6 +111,7 @@ public void close() { if ( closed.compareAndSet( false, true ) ) { + logger.trace( "Initiating connection pool termination" ); for ( BlockingPooledConnectionQueue pool : pools.values() ) { pool.terminate(); From c4a4639b991d35bc13bc4bbd1d1829313b740d1b Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 6 Jul 2017 22:25:26 +0200 Subject: [PATCH 09/10] Added `Logger#warn(String, Throwable)` And removed unused test events based infrastructure. --- .../internal/logging/DelegatingLogger.java | 6 + .../internal/logging/DevNullLogger.java | 5 + .../driver/internal/logging/JULogger.java | 6 + .../main/java/org/neo4j/driver/v1/Logger.java | 19 +- .../java/org/neo4j/driver/internal/Event.java | 64 ---- .../neo4j/driver/internal/EventHandler.java | 285 ------------------ .../driver/internal/RoutingDriverTest.java | 7 +- .../logging/DelegatingLoggerTest.java | 24 ++ .../BlockingPooledConnectionQueueTest.java | 2 +- .../neo4j/driver/internal/util/FakeClock.java | 176 +---------- .../java/org/neo4j/driver/v1/EventLogger.java | 225 -------------- 11 files changed, 72 insertions(+), 747 deletions(-) delete mode 100644 driver/src/test/java/org/neo4j/driver/internal/Event.java delete mode 100644 driver/src/test/java/org/neo4j/driver/internal/EventHandler.java delete mode 100644 driver/src/test/java/org/neo4j/driver/v1/EventLogger.java diff --git a/driver/src/main/java/org/neo4j/driver/internal/logging/DelegatingLogger.java b/driver/src/main/java/org/neo4j/driver/internal/logging/DelegatingLogger.java index ab4e24e39a..83d74e69e8 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/logging/DelegatingLogger.java +++ b/driver/src/main/java/org/neo4j/driver/internal/logging/DelegatingLogger.java @@ -56,6 +56,12 @@ public void warn( String message, Object... params ) delegate.warn( messageWithPrefix( message ), params ); } + @Override + public void warn( String message, Throwable cause ) + { + delegate.warn( messageWithPrefix( message ), cause ); + } + @Override public void debug( String message, Object... params ) { diff --git a/driver/src/main/java/org/neo4j/driver/internal/logging/DevNullLogger.java b/driver/src/main/java/org/neo4j/driver/internal/logging/DevNullLogger.java index 629baf52e2..4a65c057dc 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/logging/DevNullLogger.java +++ b/driver/src/main/java/org/neo4j/driver/internal/logging/DevNullLogger.java @@ -43,6 +43,11 @@ public void warn( String message, Object... params ) { } + @Override + public void warn( String message, Throwable cause ) + { + } + @Override public void debug( String message, Object... params ) { diff --git a/driver/src/main/java/org/neo4j/driver/internal/logging/JULogger.java b/driver/src/main/java/org/neo4j/driver/internal/logging/JULogger.java index 79fd78a8de..e1a9ccb981 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/logging/JULogger.java +++ b/driver/src/main/java/org/neo4j/driver/internal/logging/JULogger.java @@ -54,6 +54,12 @@ public void warn( String format, Object... params ) delegate.log( Level.WARNING, String.format( format, params ) ); } + @Override + public void warn( String message, Throwable cause ) + { + delegate.log( Level.WARNING, message, cause ); + } + @Override public void debug( String format, Object... params ) { diff --git a/driver/src/main/java/org/neo4j/driver/v1/Logger.java b/driver/src/main/java/org/neo4j/driver/v1/Logger.java index ac52d2668b..c9d043a6f5 100644 --- a/driver/src/main/java/org/neo4j/driver/v1/Logger.java +++ b/driver/src/main/java/org/neo4j/driver/v1/Logger.java @@ -25,6 +25,7 @@ public interface Logger { /** * Logs errors from this driver + * * @param message the error message * @param cause the cause of the error */ @@ -32,6 +33,7 @@ public interface Logger /** * Logs information from the driver + * * @param message the information message * @param params parameters used in the information message */ @@ -39,15 +41,25 @@ public interface Logger /** * Logs warnings that happened during using the driver + * * @param message the warning message * @param params parameters used in the warning message */ void warn( String message, Object... params ); + /** + * Logs warnings that happened during using the driver + * + * @param message the warning message + * @param cause the cause of the warning + */ + void warn( String message, Throwable cause ); + /** * Logs bolt messages sent and received by this driver. * It is only enabled when {@link Logger#isDebugEnabled()} returns {@code True}. * This logging level generates a lot of log entries. + * * @param message the bolt message * @param params parameters used in generating the bolt message */ @@ -57,6 +69,7 @@ public interface Logger * Logs binary sent and received by this driver. * It is only enabled when {@link Logger#isTraceEnabled()} returns {@code True}. * This logging level generates huge amount of log entries. + * * @param message the bolt message in hex * @param params parameters used in generating the hex message */ @@ -64,15 +77,17 @@ public interface Logger /** * Return true if the trace logging level is enabled. - * @see Logger#trace(String, Object...) + * * @return true if the trace logging level is enabled. + * @see Logger#trace(String, Object...) */ boolean isTraceEnabled(); /** * Return true if the debug level is enabled. - * @see Logger#debug(String, Object...) + * * @return true if the debug level is enabled. + * @see Logger#debug(String, Object...) */ boolean isDebugEnabled(); } diff --git a/driver/src/test/java/org/neo4j/driver/internal/Event.java b/driver/src/test/java/org/neo4j/driver/internal/Event.java deleted file mode 100644 index cb0acfc786..0000000000 --- a/driver/src/test/java/org/neo4j/driver/internal/Event.java +++ /dev/null @@ -1,64 +0,0 @@ -/* - * Copyright (c) 2002-2017 "Neo Technology," - * Network Engine for Objects in Lund AB [http://neotechnology.com] - * - * This file is part of Neo4j. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.neo4j.driver.internal; - -import java.io.PrintWriter; -import java.io.StringWriter; -import java.lang.reflect.Method; - -public abstract class Event -{ - final Class handlerType; - - @SuppressWarnings( "unchecked" ) - public Event() - { - this.handlerType = (Class) handlerType( getClass() ); - } - - @Override - public String toString() - { - StringWriter res = new StringWriter(); - try ( PrintWriter out = new PrintWriter( res ) ) - { - EventHandler.write( this, out ); - } - return res.toString(); - } - - public abstract void dispatch( Handler handler ); - - private static Class handlerType( Class type ) - { - for ( Class c = type; c != Object.class; c = c.getSuperclass() ) - { - for ( Method method : c.getDeclaredMethods() ) - { - if ( method.getName().equals( "dispatch" ) - && method.getParameterTypes().length == 1 - && !method.isSynthetic() ) - { - return method.getParameterTypes()[0]; - } - } - } - throw new Error( "Cannot determine Handler type from dispatch(Handler) method." ); - } -} diff --git a/driver/src/test/java/org/neo4j/driver/internal/EventHandler.java b/driver/src/test/java/org/neo4j/driver/internal/EventHandler.java deleted file mode 100644 index 11dc257ca8..0000000000 --- a/driver/src/test/java/org/neo4j/driver/internal/EventHandler.java +++ /dev/null @@ -1,285 +0,0 @@ -/* - * Copyright (c) 2002-2017 "Neo Technology," - * Network Engine for Objects in Lund AB [http://neotechnology.com] - * - * This file is part of Neo4j. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.neo4j.driver.internal; - -import org.hamcrest.Matcher; - -import java.io.PrintStream; -import java.io.PrintWriter; -import java.lang.reflect.Constructor; -import java.lang.reflect.InvocationHandler; -import java.lang.reflect.Method; -import java.lang.reflect.Proxy; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.Objects; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; -import java.util.concurrent.CopyOnWriteArrayList; - -import org.neo4j.driver.internal.util.MatcherFactory; -import org.neo4j.driver.v1.EventLogger; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.any; -import static org.hamcrest.Matchers.not; -import static org.neo4j.driver.internal.util.MatcherFactory.containsAtLeast; -import static org.neo4j.driver.internal.util.MatcherFactory.count; - -public final class EventHandler -{ - private final List events = new ArrayList<>(); - private final ConcurrentMap,List> handlers = new ConcurrentHashMap<>(); - - public void add( Event event ) - { - synchronized ( events ) - { - events.add( event ); - } - List handlers = this.handlers.get( event.handlerType ); - if ( handlers != null ) - { - for ( Object handler : handlers ) - { - try - { - dispatch( event, handler ); - } - catch ( Exception e ) - { - System.err.println( "Failed to dispatch event: " + event + " to handler: " + handler ); - e.printStackTrace( System.err ); - } - } - } - } - - @SuppressWarnings( "unchecked" ) - public void registerHandler( Class type, Handler handler ) - { - List handlers = this.handlers.get( type ); - if ( handlers == null ) - { - List candidate = new CopyOnWriteArrayList<>(); - handlers = this.handlers.putIfAbsent( type, candidate ); - if ( handlers == null ) - { - handlers = candidate; - } - } - handlers.add( handler ); - } - - @SafeVarargs - public final void assertContains( MatcherFactory... matchers ) - { - synchronized ( events ) - { - assertThat( events, containsAtLeast( (MatcherFactory[]) matchers ) ); - } - } - - @SafeVarargs - public final void assertContains( Matcher... matchers ) - { - synchronized ( events ) - { - assertThat( events, containsAtLeast( (Matcher[]) matchers ) ); - } - } - - public final void assertCount( Matcher matcher, Matcher count ) - { - synchronized ( events ) - { - assertThat( "Unexpected count " + count, events, (Matcher) count( matcher, count ) ); - } - } - - public void assertNone( Matcher matcher ) - { - synchronized ( events ) - { - assertThat( events, not( containsAtLeast( (Matcher) matcher ) ) ); - } - } - - public void printEvents( PrintStream out ) - { - printEvents( any( Event.class ), out ); - } - - public void printEvents( Matcher matcher, PrintStream out ) - { - try ( PrintWriter writer = new PrintWriter( out ) ) - { - printEvents( matcher, writer ); - } - } - - public void printEvents( PrintWriter out ) - { - printEvents( any( Event.class ), out ); - } - - public void printEvents( Matcher matcher, PrintWriter out ) - { - synchronized ( events ) - { - for ( Event event : events ) - { - if ( matcher.matches( event ) ) - { - write( event, out ); - out.println(); - } - } - } - } - - public void forEach( Object handler ) - { - synchronized ( events ) - { - for ( Event event : events ) - { - if ( event.handlerType.isInstance( handler ) ) - { - dispatch( event, handler ); - } - } - } - } - - private static void dispatch( Event event, Object handler ) - { - event.dispatch( event.handlerType.cast( handler ) ); - } - - static void write( Event event, PrintWriter out ) - { - dispatch( event, proxy( event.handlerType, new WriteHandler( out ) ) ); - } - - private static Handler proxy( Class handlerType, InvocationHandler handler ) - { - if ( handlerType.isInstance( handler ) ) - { - return handlerType.cast( handler ); - } - try - { - return handlerType.cast( proxies.get( handlerType ).newInstance( handler ) ); - } - catch ( RuntimeException e ) - { - throw e; - } - catch ( Exception e ) - { - throw new RuntimeException( e ); - } - } - - private static final ClassValue proxies = new ClassValue() - { - @Override - protected Constructor computeValue( Class type ) - { - Class proxy = Proxy.getProxyClass( type.getClassLoader(), type ); - try - { - return proxy.getConstructor( InvocationHandler.class ); - } - catch ( NoSuchMethodException e ) - { - throw new RuntimeException( e ); - } - } - }; - - private static class WriteHandler implements InvocationHandler, EventLogger.Sink - { - private final PrintWriter out; - - WriteHandler( PrintWriter out ) - { - this.out = out; - } - - @Override - public Object invoke( Object proxy, Method method, Object[] args ) throws Throwable - { - out.append( method.getName() ).append( '(' ); - String sep = " "; - for ( Object arg : args ) - { - out.append( sep ); - if ( arg == null || !arg.getClass().isArray() ) - { - out.append( Objects.toString( arg ) ); - } - else if ( arg instanceof Object[] ) - { - out.append( Arrays.toString( (Object[]) arg ) ); - } - else - { - out.append( Arrays.class.getMethod( "toString", arg.getClass() ).invoke( null, arg ).toString() ); - } - sep = ", "; - } - if ( args.length > 0 ) - { - out.append( ' ' ); - } - out.append( ')' ); - return null; - } - - @Override - public void log( String name, EventLogger.Level level, Throwable cause, String message, Object... params ) - { - out.append( level.name() ).append( ": " ); - if ( params != null ) - { - try - { - out.format( message, params ); - } - catch ( Exception e ) - { - out.format( "InvalidFormat(message=\"%s\", params=%s, failure=%s)", - message, Arrays.toString( params ), e ); - } - out.println(); - } - else - { - out.println( message ); - } - if ( cause != null ) - { - cause.printStackTrace( out ); - } - } - } -} diff --git a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java index 9f7057c792..0303f55f83 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java @@ -44,7 +44,6 @@ import org.neo4j.driver.v1.AccessMode; import org.neo4j.driver.v1.Config; import org.neo4j.driver.v1.Driver; -import org.neo4j.driver.v1.EventLogger; import org.neo4j.driver.v1.GraphDatabase; import org.neo4j.driver.v1.Logging; import org.neo4j.driver.v1.Value; @@ -65,6 +64,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.neo4j.driver.internal.cluster.ClusterCompositionProviderTest.serverInfo; +import static org.neo4j.driver.internal.logging.DevNullLogging.DEV_NULL_LOGGING; import static org.neo4j.driver.internal.security.SecurityPlan.insecure; import static org.neo4j.driver.v1.Values.value; @@ -74,9 +74,7 @@ public class RoutingDriverTest public ExpectedException exception = ExpectedException.none(); private static final BoltServerAddress SEED = new BoltServerAddress( "localhost", 7687 ); private static final String GET_SERVERS = "CALL dbms.cluster.routing.getServers"; - private final EventHandler events = new EventHandler(); - private final FakeClock clock = new FakeClock( events, true ); - private final Logging logging = EventLogger.provider( events, EventLogger.Level.TRACE ); + private final FakeClock clock = new FakeClock(); @Test public void shouldDiscoveryOnInitialization() @@ -347,6 +345,7 @@ private final Driver driverWithServers( long ttl, Map... serverIn private Driver driverWithPool( ConnectionPool pool ) { + Logging logging = DEV_NULL_LOGGING; RoutingSettings settings = new RoutingSettings( 10, 5_000, null ); ConnectionProvider connectionProvider = new LoadBalancer( SEED, settings, pool, clock, logging, new LeastConnectedLoadBalancingStrategy( pool, logging ) ); diff --git a/driver/src/test/java/org/neo4j/driver/internal/logging/DelegatingLoggerTest.java b/driver/src/test/java/org/neo4j/driver/internal/logging/DelegatingLoggerTest.java index 29dcc111bc..8a25a22a24 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/logging/DelegatingLoggerTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/logging/DelegatingLoggerTest.java @@ -137,6 +137,18 @@ public void shouldDelegateWarnMessageWhenNoPrefix() verify( delegate ).warn( MESSAGE ); } + @Test + public void shouldDelegateWarnMessageWithoutErrorWhenNoPrefix() + { + Logger delegate = newLoggerMock(); + DelegatingLogger logger = new DelegatingLogger( delegate ); + + Exception cause = new Exception(); + logger.warn( MESSAGE, cause ); + + verify( delegate ).warn( MESSAGE, cause ); + } + @Test public void shouldDelegateDebugMessageWhenNoPrefix() { @@ -192,6 +204,18 @@ public void shouldDelegateWarnMessageWithPrefix() verify( delegate ).warn( "[Output] Hello World!" ); } + @Test + public void shouldDelegateWarnMessageWithErrorWithPrefix() + { + Logger delegate = newLoggerMock(); + DelegatingLogger logger = new DelegatingLogger( delegate, PREFIX ); + + Exception cause = new Exception(); + logger.warn( MESSAGE, cause ); + + verify( delegate ).warn( "[Output] Hello World!", cause ); + } + @Test public void shouldDelegateDebugMessageWithPrefix() { diff --git a/driver/src/test/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueueTest.java b/driver/src/test/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueueTest.java index 25f77a9b89..de0945294e 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueueTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueueTest.java @@ -193,7 +193,7 @@ public void shouldLogWhenConnectionDisposeFails() queue.terminate(); - verify( logger ).error( anyString(), eq( closeError ) ); + verify( logger ).warn( anyString(), eq( closeError ) ); } @Test diff --git a/driver/src/test/java/org/neo4j/driver/internal/util/FakeClock.java b/driver/src/test/java/org/neo4j/driver/internal/util/FakeClock.java index 5d5fae04f7..9dedd10bae 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/util/FakeClock.java +++ b/driver/src/test/java/org/neo4j/driver/internal/util/FakeClock.java @@ -18,84 +18,30 @@ */ package org.neo4j.driver.internal.util; -import org.hamcrest.Description; -import org.hamcrest.Matcher; -import org.hamcrest.TypeSafeMatcher; - import java.util.concurrent.PriorityBlockingQueue; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicLongFieldUpdater; +import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.locks.LockSupport; -import org.neo4j.driver.internal.EventHandler; - -import static java.util.concurrent.atomic.AtomicLongFieldUpdater.newUpdater; -import static org.hamcrest.Matchers.any; -import static org.hamcrest.Matchers.equalTo; - public class FakeClock implements Clock { - public interface EventSink - { - EventSink VOID = new Adapter(); - - void sleep( long timestamp, long millis ); - - void progress( long millis ); - - class Adapter implements EventSink - { - @Override - public void sleep( long timestamp, long millis ) - { - } - - @Override - public void progress( long millis ) - { - } - } - } - - private final EventSink events; - @SuppressWarnings( "unused"/*assigned through AtomicLongFieldUpdater*/ ) - private volatile long timestamp; - private static final AtomicLongFieldUpdater TIMESTAMP = newUpdater( FakeClock.class, "timestamp" ); - private PriorityBlockingQueue threads; + private final AtomicLong timestamp = new AtomicLong(); + private final PriorityBlockingQueue threads; public FakeClock() { - this( (EventHandler) null, false ); - } - - public FakeClock( final EventHandler events, boolean progressOnSleep ) - { - this( events == null ? null : new EventSink() - { - @Override - public void sleep( long timestamp, long duration ) - { - events.add( new Event.Sleep( Thread.currentThread(), timestamp, duration ) ); - } - - @Override - public void progress( long timestamp ) - { - events.add( new Event.Progress( Thread.currentThread(), timestamp ) ); - } - }, progressOnSleep ); + this( false ); } - public FakeClock( EventSink events, boolean progressOnSleep ) + private FakeClock( boolean progressOnSleep ) { - this.events = events == null ? EventSink.VOID : events; this.threads = progressOnSleep ? null : new PriorityBlockingQueue(); } @Override public long millis() { - return timestamp; + return timestamp.get(); } @Override @@ -105,8 +51,7 @@ public void sleep( long millis ) { return; } - long target = timestamp + millis; - events.sleep( target - millis, millis ); + long target = timestamp.get() + millis; if ( threads == null ) { progress( millis ); @@ -118,7 +63,7 @@ public void sleep( long millis ) threads.add( token ); for ( ; ; ) { - if ( timestamp >= target ) + if ( timestamp.get() >= target ) { threads.remove( token ); return; @@ -135,13 +80,13 @@ public void progress( long millis ) { throw new IllegalArgumentException( "time can only progress forwards" ); } - events.progress( TIMESTAMP.addAndGet( this, millis ) ); + timestamp.addAndGet( millis ); if ( threads != null ) { // wake up the threads that are sleeping awaiting the current time for ( WaitingThread thread; (thread = threads.peek()) != null; ) { - if ( thread.timestamp < timestamp ) + if ( thread.timestamp < timestamp.get() ) { threads.remove( thread ); LockSupport.unpark( thread.thread ); @@ -150,107 +95,6 @@ public void progress( long millis ) } } - public static abstract class Event extends org.neo4j.driver.internal.Event - { - final Thread thread; - - private Event( Thread thread ) - { - this.thread = thread; - } - - public static Matcher sleep( long duration ) - { - return sleep( any( Thread.class ), any( Long.class ), equalTo( duration ) ); - } - - public static Matcher sleep( - final Matcher thread, - final Matcher timestamp, - final Matcher duration ) - { - return new TypeSafeMatcher() - { - @Override - public void describeTo( Description description ) - { - description.appendText( "Sleep Event on thread <" ) - .appendDescriptionOf( thread ) - .appendText( "> at timestamp " ) - .appendDescriptionOf( timestamp ) - .appendText( " for duration " ) - .appendDescriptionOf( timestamp ) - .appendText( " (in milliseconds)" ); - } - - @Override - protected boolean matchesSafely( Sleep event ) - { - return thread.matches( event.thread ) - && timestamp.matches( event.timestamp ) - && duration.matches( event.duration ); - } - }; - } - - public static Matcher progress( final Matcher thread, final Matcher timestamp ) - { - return new TypeSafeMatcher() - { - @Override - public void describeTo( Description description ) - { - description.appendText( "Time progresses to timestamp " ) - .appendDescriptionOf( timestamp ) - .appendText( " by thread <" ) - .appendDescriptionOf( thread ) - .appendText( ">" ); - } - - @Override - protected boolean matchesSafely( Progress event ) - { - return thread.matches( event.thread ) && timestamp.matches( event.timestamp ); - } - }; - } - - private static class Sleep extends Event - { - private final long timestamp, duration; - - Sleep( Thread thread, long timestamp, long duration ) - { - super( thread ); - this.timestamp = timestamp; - this.duration = duration; - } - - @Override - public void dispatch( EventSink sink ) - { - sink.sleep( timestamp, duration ); - } - } - - private static class Progress extends Event - { - private final long timestamp; - - Progress( Thread thread, long timestamp ) - { - super( thread ); - this.timestamp = timestamp; - } - - @Override - public void dispatch( EventSink sink ) - { - sink.progress( timestamp ); - } - } - } - private static class WaitingThread { final Thread thread; diff --git a/driver/src/test/java/org/neo4j/driver/v1/EventLogger.java b/driver/src/test/java/org/neo4j/driver/v1/EventLogger.java deleted file mode 100644 index e3dd9efc82..0000000000 --- a/driver/src/test/java/org/neo4j/driver/v1/EventLogger.java +++ /dev/null @@ -1,225 +0,0 @@ -/* - * Copyright (c) 2002-2017 "Neo Technology," - * Network Engine for Objects in Lund AB [http://neotechnology.com] - * - * This file is part of Neo4j. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.neo4j.driver.v1; - -import org.hamcrest.Description; -import org.hamcrest.Matcher; -import org.hamcrest.TypeSafeMatcher; - -import org.neo4j.driver.internal.Event; -import org.neo4j.driver.internal.EventHandler; - -import static java.util.Objects.requireNonNull; - -public class EventLogger implements Logger -{ - public static Logging provider( EventHandler events, Level level ) - { - return provider( sink( requireNonNull( events, "events" ) ), level ); - } - - public static Logging provider( final Sink events, final Level level ) - { - requireNonNull( events, "events" ); - requireNonNull( level, "level" ); - return new Logging() - { - @Override - public Logger getLog( String name ) - { - return new EventLogger( events, name, level ); - } - }; - } - - public interface Sink - { - void log( String name, Level level, Throwable cause, String message, Object... params ); - } - - private final boolean debug, trace; - private final Sink events; - private final String name; - - public EventLogger( EventHandler events, String name, Level level ) - { - this( sink( requireNonNull( events, "events" ) ), name, level ); - } - - public EventLogger( Sink events, String name, Level level ) - { - this.events = requireNonNull( events, "events" ); - this.name = name; - level = requireNonNull( level, "level" ); - this.debug = Level.DEBUG.compareTo( level ) <= 0; - this.trace = Level.TRACE.compareTo( level ) <= 0; - } - - private static Sink sink( final EventHandler events ) - { - return new Sink() - { - @Override - public void log( String name, Level level, Throwable cause, String message, Object... params ) - { - events.add( new Entry( Thread.currentThread(), name, level, cause, message, params ) ); - } - }; - } - - public enum Level - { - ERROR, - WARN, - INFO, - DEBUG, - TRACE - } - - @Override - public void error( String message, Throwable cause ) - { - events.log( name, Level.ERROR, cause, message ); - } - - @Override - public void info( String message, Object... params ) - { - events.log( name, Level.INFO, null, message, params ); - } - - @Override - public void warn( String message, Object... params ) - { - events.log( name, Level.WARN, null, message, params ); - } - - @Override - public void debug( String message, Object... params ) - { - events.log( name, Level.DEBUG, null, message, params ); - } - - @Override - public void trace( String message, Object... params ) - { - events.log( name, Level.TRACE, null, message, params ); - } - - @Override - public boolean isTraceEnabled() - { - return trace; - } - - @Override - public boolean isDebugEnabled() - { - return debug; - } - - public static final class Entry extends Event - { - private final Thread thread; - private final String name; - private final Level level; - private final Throwable cause; - private final String message; - private final Object[] params; - - private Entry( Thread thread, String name, Level level, Throwable cause, String message, Object... params ) - { - this.thread = thread; - this.name = name; - this.level = requireNonNull( level, "level" ); - this.cause = cause; - this.message = message; - this.params = params; - } - - @Override - public void dispatch( Sink sink ) - { - sink.log( name, level, cause, message, params ); - } - - private String formatted() - { - return params == null ? message : String.format( message, params ); - } - - public static Matcher logEntry( - final Matcher thread, - final Matcher name, - final Matcher level, - final Matcher cause, - final Matcher message, - final Matcher params ) - { - return new TypeSafeMatcher() - { - @Override - protected boolean matchesSafely( Entry entry ) - { - return level.matches( entry.level ) && - thread.matches( entry.thread ) && - name.matches( entry.name ) && - cause.matches( entry.cause ) && - message.matches( entry.message ) && - params.matches( entry.params ); - } - - @Override - public void describeTo( Description description ) - { - description.appendText( "Log entry where level " ) - .appendDescriptionOf( level ) - .appendText( " name <" ) - .appendDescriptionOf( name ) - .appendText( "> cause <" ) - .appendDescriptionOf( cause ) - .appendText( "> message <" ) - .appendDescriptionOf( message ) - .appendText( "> and parameters <" ) - .appendDescriptionOf( params ) - .appendText( ">" ); - } - }; - } - - public static Matcher message( final Matcher level, final Matcher message ) - { - return new TypeSafeMatcher() - { - @Override - protected boolean matchesSafely( Entry entry ) - { - return level.matches( entry.level ) && message.matches( entry.formatted() ); - } - - @Override - public void describeTo( Description description ) - { - description.appendText( "Log entry where level " ).appendDescriptionOf( level ) - .appendText( " and formatted message " ).appendDescriptionOf( message ); - } - }; - } - } -} From e41cab2f1779487a2e57e3e49fef88abdca62d62 Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 6 Jul 2017 22:26:27 +0200 Subject: [PATCH 10/10] Change connection dispose logging from warn --- .../internal/net/pooling/BlockingPooledConnectionQueue.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java b/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java index a17dc2a4c8..1eddbc20f6 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java +++ b/driver/src/main/java/org/neo4j/driver/internal/net/pooling/BlockingPooledConnectionQueue.java @@ -163,7 +163,7 @@ private void disposeSafely( PooledConnection connection ) } catch ( Throwable disposeError ) { - logger.error( "Error disposing connection", disposeError ); + logger.warn( "Error disposing connection during termination", disposeError ); } }