Commit d86fc293 authored by Anton Persson's avatar Anton Persson Committed by Anton Persson
Browse files

Max trip count limit feature toggle moved to static field

The cost of looking up the feature toggle in constructor each time
was noticeable in benchmarks and caused a regression.
parent 0e92afb9
Showing with 47 additions and 84 deletions
+47 -84
...@@ -45,17 +45,16 @@ import org.neo4j.util.FeatureToggles; ...@@ -45,17 +45,16 @@ import org.neo4j.util.FeatureToggles;
*/ */
public class TripCountingRootCatchup implements RootCatchup public class TripCountingRootCatchup implements RootCatchup
{ {
static final String MAX_TRIP_COUNT_NAME = "max_trip_count"; private static final String MAX_TRIP_COUNT_NAME = "max_trip_count";
static final int MAX_TRIP_COUNT_DEFAULT = 10; private static final int MAX_TRIP_COUNT_DEFAULT = 10;
static final int MAX_TRIP_COUNT = FeatureToggles.getInteger( TripCountingRootCatchup.class, MAX_TRIP_COUNT_NAME, MAX_TRIP_COUNT_DEFAULT );
private final Supplier<Root> rootSupplier; private final Supplier<Root> rootSupplier;
private final int maxTripCount;
private long lastFromId = TreeNode.NO_NODE_FLAG; private long lastFromId = TreeNode.NO_NODE_FLAG;
private int tripCount; private int tripCount;
TripCountingRootCatchup( Supplier<Root> rootSupplier ) TripCountingRootCatchup( Supplier<Root> rootSupplier )
{ {
this.rootSupplier = rootSupplier; this.rootSupplier = rootSupplier;
this.maxTripCount = FeatureToggles.getInteger( TripCountingRootCatchup.class, MAX_TRIP_COUNT_NAME, MAX_TRIP_COUNT_DEFAULT );
} }
@Override @Override
...@@ -81,7 +80,7 @@ public class TripCountingRootCatchup implements RootCatchup ...@@ -81,7 +80,7 @@ public class TripCountingRootCatchup implements RootCatchup
private void assertTripCount() private void assertTripCount()
{ {
if ( tripCount >= maxTripCount ) if ( tripCount >= MAX_TRIP_COUNT )
{ {
throw new TreeInconsistencyException( throw new TreeInconsistencyException(
"Index traversal aborted due to being stuck in infinite loop. This is most likely caused by an inconsistency in the index. " + "Index traversal aborted due to being stuck in infinite loop. This is most likely caused by an inconsistency in the index. " +
......
...@@ -77,7 +77,6 @@ import org.neo4j.test.rule.PageCacheRule; ...@@ -77,7 +77,6 @@ import org.neo4j.test.rule.PageCacheRule;
import org.neo4j.test.rule.RandomRule; import org.neo4j.test.rule.RandomRule;
import org.neo4j.test.rule.TestDirectory; import org.neo4j.test.rule.TestDirectory;
import org.neo4j.test.rule.fs.DefaultFileSystemRule; import org.neo4j.test.rule.fs.DefaultFileSystemRule;
import org.neo4j.util.FeatureToggles;
import static java.lang.Long.MAX_VALUE; import static java.lang.Long.MAX_VALUE;
import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.containsString;
...@@ -1668,62 +1667,54 @@ public class GBPTreeTest ...@@ -1668,62 +1667,54 @@ public class GBPTreeTest
@Test( timeout = 5_000L ) @Test( timeout = 5_000L )
public void mustThrowIfStuckInInfiniteRootCatchupMultipleConcurrentSeekers() throws IOException, InterruptedException public void mustThrowIfStuckInInfiniteRootCatchupMultipleConcurrentSeekers() throws IOException, InterruptedException
{ {
FeatureToggles.set( TripCountingRootCatchup.class, TripCountingRootCatchup.MAX_TRIP_COUNT_NAME, 10000 ); List<Long> trace = new ArrayList<>();
try MutableBoolean onOffSwitch = new MutableBoolean( true );
PageCursorTracer pageCursorTracer = trackingPageCursorTracer( trace, onOffSwitch );
PageCache pageCache = pageCacheWithTrace( pageCursorTracer );
// Build a tree with root and two children.
try ( GBPTree<MutableLong,MutableLong> tree = index( pageCache ).build() )
{ {
List<Long> trace = new ArrayList<>(); // Insert data until we have a split in root
MutableBoolean onOffSwitch = new MutableBoolean( true ); treeWithRootSplit( trace, tree );
PageCursorTracer pageCursorTracer = trackingPageCursorTracer( trace, onOffSwitch ); long leftChild = trace.get( 1 );
PageCache pageCache = pageCacheWithTrace( pageCursorTracer ); long rightChild = trace.get( 2 );
// Build a tree with root and two children. // Stop trace tracking because we will soon start pinning pages from different threads
try ( GBPTree<MutableLong,MutableLong> tree = index( pageCache ).build() ) onOffSwitch.setFalse();
// Corrupt the child
corruptTheChild( pageCache, leftChild );
corruptTheChild( pageCache, rightChild );
// When seek end up in this corrupt child we should eventually fail with a tree inconsistency exception
// even if we have multiple seeker that traverse different part of the tree and both get stuck in start from root loop.
ExecutorService executor = Executors.newFixedThreadPool( 2 );
CountDownLatch go = new CountDownLatch( 2 );
Future<Object> execute1 = executor.submit( () ->
{ {
// Insert data until we have a split in root go.countDown();
treeWithRootSplit( trace, tree ); go.await();
long leftChild = trace.get( 1 ); try ( RawCursor<Hit<MutableLong,MutableLong>,IOException> seek = tree.seek( new MutableLong( 0 ), new MutableLong( 0 ) ) )
long rightChild = trace.get( 2 );
// Stop trace tracking because we will soon start pinning pages from different threads
onOffSwitch.setFalse();
// Corrupt the child
corruptTheChild( pageCache, leftChild );
corruptTheChild( pageCache, rightChild );
// When seek end up in this corrupt child we should eventually fail with a tree inconsistency exception
// even if we have multiple seeker that traverse different part of the tree and both get stuck in start from root loop.
ExecutorService executor = Executors.newFixedThreadPool( 2 );
CountDownLatch go = new CountDownLatch( 2 );
Future<Object> execute1 = executor.submit( () ->
{ {
go.countDown(); seek.next();
go.await(); }
try ( RawCursor<Hit<MutableLong,MutableLong>,IOException> seek = tree.seek( new MutableLong( 0 ), new MutableLong( 0 ) ) ) return null;
{ } );
seek.next();
}
return null;
} );
Future<Object> execute2 = executor.submit( () -> Future<Object> execute2 = executor.submit( () ->
{
go.countDown();
go.await();
try ( RawCursor<Hit<MutableLong,MutableLong>,IOException> seek = tree.seek( new MutableLong( MAX_VALUE ), new MutableLong( MAX_VALUE ) ) )
{ {
go.countDown(); seek.next();
go.await(); }
try ( RawCursor<Hit<MutableLong,MutableLong>,IOException> seek = tree.seek( new MutableLong( MAX_VALUE ), new MutableLong( MAX_VALUE ) ) ) return null;
{ } );
seek.next();
}
return null;
} );
assertFutureFailsWithTreeInconsistencyException( execute1 ); assertFutureFailsWithTreeInconsistencyException( execute1 );
assertFutureFailsWithTreeInconsistencyException( execute2 ); assertFutureFailsWithTreeInconsistencyException( execute2 );
}
}
finally
{
FeatureToggles.clear( TripCountingRootCatchup.class, TripCountingRootCatchup.MAX_TRIP_COUNT_NAME );
} }
} }
......
...@@ -23,11 +23,8 @@ import org.junit.Test; ...@@ -23,11 +23,8 @@ import org.junit.Test;
import java.util.function.Supplier; import java.util.function.Supplier;
import org.neo4j.util.FeatureToggles;
import static org.junit.Assert.assertSame; import static org.junit.Assert.assertSame;
import static org.junit.Assert.fail; import static org.junit.Assert.fail;
import static org.neo4j.index.internal.gbptree.TripCountingRootCatchup.MAX_TRIP_COUNT_NAME;
public class TripCountingRootCatchupTest public class TripCountingRootCatchupTest
{ {
...@@ -40,7 +37,7 @@ public class TripCountingRootCatchupTest ...@@ -40,7 +37,7 @@ public class TripCountingRootCatchupTest
// When // When
try try
{ {
for ( int i = 0; i < TripCountingRootCatchup.MAX_TRIP_COUNT_DEFAULT; i++ ) for ( int i = 0; i < TripCountingRootCatchup.MAX_TRIP_COUNT; i++ )
{ {
tripCountingRootCatchup.catchupFrom( 10 ); tripCountingRootCatchup.catchupFrom( 10 );
} }
...@@ -59,7 +56,7 @@ public class TripCountingRootCatchupTest ...@@ -59,7 +56,7 @@ public class TripCountingRootCatchupTest
TripCountingRootCatchup tripCountingRootCatchup = getTripCounter(); TripCountingRootCatchup tripCountingRootCatchup = getTripCounter();
// When // When
for ( int i = 0; i < TripCountingRootCatchup.MAX_TRIP_COUNT_DEFAULT * 4; i++ ) for ( int i = 0; i < TripCountingRootCatchup.MAX_TRIP_COUNT * 4; i++ )
{ {
tripCountingRootCatchup.catchupFrom( i % 2 ); tripCountingRootCatchup.catchupFrom( i % 2 );
} }
...@@ -82,30 +79,6 @@ public class TripCountingRootCatchupTest ...@@ -82,30 +79,6 @@ public class TripCountingRootCatchupTest
assertSame( expectedRoot, actualRoot ); assertSame( expectedRoot, actualRoot );
} }
@Test
public void mustObeyFeatureToggle()
{
int configuredMaxTripCount = 5;
FeatureToggles.set( TripCountingRootCatchup.class, MAX_TRIP_COUNT_NAME, configuredMaxTripCount );
try
{
TripCountingRootCatchup tripCountingRootCatchup = getTripCounter();
for ( int i = 0; i < configuredMaxTripCount; i++ )
{
tripCountingRootCatchup.catchupFrom( 10 );
}
fail( "Expected to throw" );
}
catch ( TreeInconsistencyException e )
{
// Then good
}
finally
{
FeatureToggles.clear( TripCountingRootCatchup.class, MAX_TRIP_COUNT_NAME );
}
}
private TripCountingRootCatchup getTripCounter() private TripCountingRootCatchup getTripCounter()
{ {
Root root = new Root( 1, 2 ); Root root = new Root( 1, 2 );
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment