Skip to content

Commit d8b6c43

Browse files
pontusmelkepontusmelke
committed
Rediscovery no longer additive
Whenever we go down to the database to call `getServers` we should use the values returned from the call and forget about any servers returned in previous calls.
1 parent 6eb2dd7 commit d8b6c43

File tree

5 files changed

+112
-62
lines changed

5 files changed

+112
-62
lines changed

‎driver/src/main/java/org/neo4j/driver/internal/ClusterDriver.java‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import java.util.Collections;
2222
import java.util.Comparator;
23+
import java.util.HashSet;
2324
import java.util.List;
2425
import java.util.Set;
2526

@@ -96,16 +97,31 @@ private void checkServers()
9697
}
9798
}
9899

100+
private Set<BoltServerAddress> forgetAllServers()
101+
{
102+
final Set<BoltServerAddress> seen = new HashSet<>( );
103+
seen.addAll( routingServers );
104+
seen.addAll( readServers );
105+
seen.addAll( writeServers );
106+
routingServers.clear();
107+
readServers.clear();
108+
writeServers.clear();
109+
return seen;
110+
}
111+
99112
//must be called from a synchronized block
100113
private void getServers()
101114
{
102115
BoltServerAddress address = null;
103116
try
104117
{
105118
boolean success = false;
106-
while ( !routingServers.isEmpty() && !success )
119+
120+
ConcurrentRoundRobinSet<BoltServerAddress> routers = new ConcurrentRoundRobinSet<>( routingServers );
121+
final Set<BoltServerAddress> seen = forgetAllServers();
122+
while ( !routers.isEmpty() && !success )
107123
{
108-
address = routingServers.hop();
124+
address = routers.hop();
109125
success = call( address, GET_SERVERS, new Consumer<Record>()
110126
{
111127
@Override
@@ -115,6 +131,7 @@ public void accept( Record record )
115131
List<ServerInfo> servers = servers( record );
116132
for ( ServerInfo server : servers )
117133
{
134+
seen.removeAll( server.addresses() );
118135
switch ( server.role() )
119136
{
120137
case "READ":
@@ -135,6 +152,12 @@ public void accept( Record record )
135152
{
136153
throw new ServiceUnavailableException( "Run out of servers" );
137154
}
155+
156+
//the server no longer think we should care about these
157+
for ( BoltServerAddress remove : seen )
158+
{
159+
connections.purge( remove );
160+
}
138161
}
139162
catch ( ClientException ex )
140163
{

‎driver/src/main/java/org/neo4j/driver/internal/util/ConcurrentRoundRobinSet.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,12 @@ public ConcurrentRoundRobinSet( Comparator<T> comparator )
4343
set = new ConcurrentSkipListSet<>( comparator );
4444
}
4545

46+
public ConcurrentRoundRobinSet(ConcurrentRoundRobinSet<T> original)
47+
{
48+
set = new ConcurrentSkipListSet<>( original.set.comparator() );
49+
set.addAll( original );
50+
}
51+
4652
public T hop()
4753
{
4854
if ( current == null )

‎driver/src/test/java/org/neo4j/driver/internal/ClusterDriverStubTest.java‎

Lines changed: 46 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
/**
22
* Copyright (c) 2002-2016 "Neo Technology,"
33
* Network Engine for Objects in Lund AB [http://neotechnology.com]
4-
*
4+
* <p>
55
* This file is part of Neo4j.
6-
*
6+
* <p>
77
* Licensed under the Apache License, Version 2.0 (the "License");
88
* you may not use this file except in compliance with the License.
99
* You may obtain a copy of the License at
10-
*
11-
* http://www.apache.org/licenses/LICENSE-2.0
12-
*
10+
* <p>
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
* <p>
1313
* Unless required by applicable law or agreed to in writing, software
1414
* distributed under the License is distributed on an "AS IS" BASIS,
1515
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -46,13 +46,16 @@
4646
import org.neo4j.driver.v1.util.Function;
4747
import org.neo4j.driver.v1.util.StubServer;
4848

49+
import static org.hamcrest.Matchers.containsInAnyOrder;
4950
import static org.hamcrest.Matchers.hasItem;
5051
import static org.hamcrest.Matchers.hasSize;
5152
import static org.hamcrest.core.IsEqual.equalTo;
5253
import static org.hamcrest.core.IsNot.not;
54+
import static org.junit.Assert.assertFalse;
5355
import static org.junit.Assert.assertThat;
5456
import static org.junit.Assert.assertTrue;
5557
import static org.junit.Assert.fail;
58+
5659
@Ignore
5760
public class ClusterDriverStubTest
5861
{
@@ -73,10 +76,7 @@ public void shouldDiscoverServers() throws IOException, InterruptedException, St
7376
{
7477
// Then
7578
Set<BoltServerAddress> addresses = driver.routingServers();
76-
assertThat( addresses, hasSize( 3 ) );
77-
assertThat( addresses, hasItem( new BoltServerAddress( "127.0.0.1", 9001 ) ) );
78-
assertThat( addresses, hasItem( new BoltServerAddress( "127.0.0.1", 9002 ) ) );
79-
assertThat( addresses, hasItem( new BoltServerAddress( "127.0.0.1", 9003 ) ) );
79+
assertThat( addresses, containsInAnyOrder( address(9001), address( 9002 ), address( 9003 ) ) );
8080
}
8181

8282
// Finally
@@ -89,23 +89,21 @@ public void shouldDiscoverNewServers() throws IOException, InterruptedException,
8989
// Given
9090
StubServer server = StubServer.start( resource( "discover_new_servers.script" ), 9001 );
9191
URI uri = URI.create( "bolt+routing://127.0.0.1:9001" );
92+
BoltServerAddress seed = address( 9001 );
9293

9394
// When
9495
try ( ClusterDriver driver = (ClusterDriver) GraphDatabase.driver( uri, config ) )
9596
{
9697
// Then
9798
Set<BoltServerAddress> addresses = driver.routingServers();
98-
assertThat( addresses, hasSize( 4 ) );
99-
assertThat( addresses, hasItem( new BoltServerAddress( "127.0.0.1", 9001 ) ) );
100-
assertThat( addresses, hasItem( new BoltServerAddress( "127.0.0.1", 9002 ) ) );
101-
assertThat( addresses, hasItem( new BoltServerAddress( "127.0.0.1", 9003 ) ) );
102-
assertThat( addresses, hasItem( new BoltServerAddress( "127.0.0.1", 9004 ) ) );
99+
assertThat( addresses, containsInAnyOrder( address(9002), address( 9003 ), address( 9004 ) ) );
103100
}
104101

105102
// Finally
106103
assertThat( server.exitStatus(), equalTo( 0 ) );
107104
}
108105

106+
109107
@Test
110108
public void shouldHandleEmptyResponse() throws IOException, InterruptedException, StubServer.ForceKilled
111109
{
@@ -115,8 +113,8 @@ public void shouldHandleEmptyResponse() throws IOException, InterruptedException
115113
try ( ClusterDriver driver = (ClusterDriver) GraphDatabase.driver( uri, config ) )
116114
{
117115
Set<BoltServerAddress> servers = driver.routingServers();
118-
assertThat( servers, hasSize( 1 ) );
119-
assertThat( servers, hasItem( new BoltServerAddress( "127.0.0.1", 9001 ) ) );
116+
assertThat( servers, hasSize( 0 ) );
117+
assertFalse( driver.connectionPool().hasAddress( address( 9001 ) ) );
120118
}
121119

122120
// Finally
@@ -265,7 +263,7 @@ public void shouldRoundRobinWriteSessions() throws IOException, InterruptedExcep
265263
{
266264
for ( int i = 0; i < 2; i++ )
267265
{
268-
try(Session session = driver.session() )
266+
try ( Session session = driver.session() )
269267
{
270268
session.run( "CREATE (n {name:'Bob'})" );
271269
}
@@ -291,15 +289,9 @@ public void shouldRememberEndpoints() throws IOException, InterruptedException,
291289
{
292290
session.run( "MATCH (n) RETURN n.name" ).consume();
293291

294-
assertThat( driver.readServers(), hasSize( 2 ));
295-
assertThat( driver.readServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9005 ) ) );
296-
assertThat( driver.readServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9006 ) ) );
297-
assertThat( driver.writeServers(), hasSize( 2 ));
298-
assertThat( driver.writeServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9007 ) ) );
299-
assertThat( driver.writeServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9008 ) ) );
300-
//Make sure we don't cache acquired servers as discovery servers
301-
assertThat( driver.routingServers(), not(hasItem( new BoltServerAddress( "127.0.0.1", 9005 ))));
302-
assertThat( driver.routingServers(), not(hasItem( new BoltServerAddress( "127.0.0.1", 9006 ))));
292+
assertThat( driver.readServers(), containsInAnyOrder( address( 9005 ), address( 9006 ) ) );
293+
assertThat( driver.writeServers(), containsInAnyOrder( address( 9007 ), address( 9008 ) ) );
294+
assertThat( driver.routingServers(), containsInAnyOrder( address( 9001 ), address( 9002 ), address( 9003 ) ) );
303295
}
304296
// Finally
305297
assertThat( server.exitStatus(), equalTo( 0 ) );
@@ -316,29 +308,30 @@ public void shouldForgetEndpointsOnFailure() throws IOException, InterruptedExce
316308
StubServer.start( resource( "dead_server.script" ), 9005 );
317309
URI uri = URI.create( "bolt+routing://127.0.0.1:9001" );
318310
ClusterDriver driver = (ClusterDriver) GraphDatabase.driver( uri, config );
319-
boolean failed = false;
320311
try
321312
{
322313
Session session = driver.session( AccessRole.READ );
323314
session.run( "MATCH (n) RETURN n.name" ).consume();
324315
session.close();
316+
fail();
325317
}
326318
catch ( SessionExpiredException e )
327319
{
328-
failed = true;
320+
//ignore
329321
}
330322

331-
assertTrue( failed );
332-
assertThat( driver.readServers(), not(hasItem( new BoltServerAddress( "127.0.0.1", 9005 ) ) ));
323+
assertThat( driver.readServers(), not( hasItem( address( 9005 ) ) ) );
333324
assertThat( driver.writeServers(), hasSize( 2 ) );
325+
assertFalse( driver.connectionPool().hasAddress( address( 9005 ) ) );
334326
driver.close();
335327

336328
// Finally
337329
assertThat( server.exitStatus(), equalTo( 0 ) );
338330
}
339331

340332
@Test
341-
public void shouldRediscoverIfNecessaryOnSessionAcquisition() throws IOException, InterruptedException, StubServer.ForceKilled
333+
public void shouldRediscoverIfNecessaryOnSessionAcquisition()
334+
throws IOException, InterruptedException, StubServer.ForceKilled
342335
{
343336
// Given
344337
StubServer server = StubServer.start( resource( "rediscover.script" ), 9001 );
@@ -349,17 +342,15 @@ public void shouldRediscoverIfNecessaryOnSessionAcquisition() throws IOException
349342

350343
//On creation we only find ourselves
351344
ClusterDriver driver = (ClusterDriver) GraphDatabase.driver( uri, config );
352-
assertThat( driver.routingServers(), hasSize( 1 ) );
353-
assertThat( driver.routingServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9001 ) ));
345+
assertThat( driver.routingServers(), containsInAnyOrder( address( 9001 ) ) );
346+
assertTrue( driver.connectionPool().hasAddress( address( 9001 ) ) );
354347

355-
//since we know about less than three servers a rediscover should be triggered
348+
//since we have no write nor read servers we must rediscover
356349
Session session = driver.session( AccessRole.READ );
357-
assertThat( driver.routingServers(), hasSize( 4 ) );
358-
assertThat( driver.routingServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9001 ) ));
359-
assertThat( driver.routingServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9002 ) ));
360-
assertThat( driver.routingServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9003 ) ));
361-
assertThat( driver.routingServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9004 ) ));
362-
350+
assertThat( driver.routingServers(), containsInAnyOrder(address( 9002 ),
351+
address( 9003 ), address( 9004 ) ) );
352+
//server told os to forget 9001
353+
assertFalse( driver.connectionPool().hasAddress( address( 9001 ) ) );
363354
session.close();
364355
driver.close();
365356

@@ -379,8 +370,7 @@ public void shouldOnlyGetServersOnce() throws IOException, InterruptedException,
379370

380371
//On creation we only find ourselves
381372
final ClusterDriver driver = (ClusterDriver) GraphDatabase.driver( uri, config );
382-
assertThat( driver.routingServers(), hasSize( 1 ) );
383-
assertThat( driver.routingServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9001 ) ));
373+
assertThat( driver.routingServers(), containsInAnyOrder( address( 9001 ) ) );
384374

385375
ExecutorService runner = Executors.newFixedThreadPool( 10 );
386376
for ( int i = 0; i < 10; i++ )
@@ -391,7 +381,7 @@ public void shouldOnlyGetServersOnce() throws IOException, InterruptedException,
391381
public void run()
392382
{
393383
//noinspection EmptyTryBlock
394-
try(Session ignore = driver.session( AccessRole.READ ))
384+
try ( Session ignore = driver.session( AccessRole.READ ) )
395385
{
396386
//empty
397387
}
@@ -401,11 +391,7 @@ public void run()
401391
}
402392
runner.awaitTermination( 10, TimeUnit.SECONDS );
403393
//since we know about less than three servers a rediscover should be triggered
404-
assertThat( driver.routingServers(), hasSize( 4 ) );
405-
assertThat( driver.routingServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9001 ) ));
406-
assertThat( driver.routingServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9002 ) ));
407-
assertThat( driver.routingServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9003 ) ));
408-
assertThat( driver.routingServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9004 ) ));
394+
assertThat( driver.routingServers(), containsInAnyOrder( address( 9002 ), address( 9003 ), address( 9004 ) ) );
409395

410396
driver.close();
411397

@@ -450,19 +436,19 @@ public void shouldHandleLeaderSwitchWhenWriting()
450436
boolean failed = false;
451437
try ( Session session = driver.session( AccessRole.WRITE ) )
452438
{
453-
assertThat(driver.writeServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9007 ) ));
454-
assertThat(driver.writeServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9008 ) ));
439+
assertThat( driver.writeServers(), hasItem(address( 9007 ) ) );
440+
assertThat( driver.writeServers(), hasItem( address( 9008 ) ) );
455441
session.run( "CREATE ()" ).consume();
456442
}
457-
catch (SessionExpiredException e)
443+
catch ( SessionExpiredException e )
458444
{
459445
failed = true;
460-
assertThat(e.getMessage(), equalTo( "Server at 127.0.0.1:9007 no longer accepts writes" ));
446+
assertThat( e.getMessage(), equalTo( "Server at 127.0.0.1:9007 no longer accepts writes" ) );
461447
}
462448
assertTrue( failed );
463-
assertThat( driver.writeServers(), not( hasItem( new BoltServerAddress( "127.0.0.1", 9007 ) ) ) );
464-
assertThat( driver.writeServers(), hasItem( new BoltServerAddress( "127.0.0.1", 9008 ) ) );
465-
assertTrue( driver.connectionPool().hasAddress( new BoltServerAddress( "127.0.0.1", 9007 ) ) );
449+
assertThat( driver.writeServers(), not( hasItem( address( 9007 ) ) ) );
450+
assertThat( driver.writeServers(), hasItem( address( 9008 ) ) );
451+
assertTrue( driver.connectionPool().hasAddress( address( 9007 ) ) );
466452

467453
driver.close();
468454
// Finally
@@ -478,4 +464,9 @@ String resource( String fileName )
478464
}
479465
return resource.getFile();
480466
}
467+
468+
private BoltServerAddress address( int port )
469+
{
470+
return new BoltServerAddress( "127.0.0.1", port );
471+
}
481472
}

0 commit comments

Comments
 (0)