Address PR comments, various cleanups

neo4j · Mar 14, 2017 · 5fff2a2 · 5fff2a2
1 parent 191c10c
commit 5fff2a2
Show file tree

Hide file tree

Showing 6 changed files with 17 additions and 21 deletions.
diff --git a/community/index/src/main/java/org/neo4j/index/internal/gbptree/CrashGenCleaner.java b/community/index/src/main/java/org/neo4j/index/internal/gbptree/CrashGenCleaner.java
@@ -47,9 +47,9 @@ class CrashGenCleaner
     private final long stableGeneration;
     private final long unstableGeneration;
     private final Monitor monitor;
-    private final long maxKeyCount;
+    private final long internalMaxKeyCount;
 
-    public CrashGenCleaner( PagedFile pagedFile, TreeNode<?,?> treeNode, long lowTreeNodeId, long highTreeNodeId,
+    CrashGenCleaner( PagedFile pagedFile, TreeNode<?,?> treeNode, long lowTreeNodeId, long highTreeNodeId,
             long stableGeneration, long unstableGeneration, Monitor monitor )
     {
         this.pagedFile = pagedFile;
@@ -59,7 +59,7 @@ public CrashGenCleaner( PagedFile pagedFile, TreeNode<?,?> treeNode, long lowTre
         this.stableGeneration = stableGeneration;
         this.unstableGeneration = unstableGeneration;
         this.monitor = monitor;
-        this.maxKeyCount = treeNode.internalMaxKeyCount();
+        this.internalMaxKeyCount = treeNode.internalMaxKeyCount();
     }
 
     // === Methods about the execution and threading ===
@@ -130,11 +130,6 @@ private Runnable cleaner( AtomicLong nextId, AtomicReference<Throwable> error, A
                             break;
                         }
 
-                        // TODO: we should try to figure out if the disk backing this paged file
-                        // is good at concurrent random reads (i.e. if it's an SSD).
-                        // If it's not it'd likely be beneficial to let other threads wait until
-                        // goTo completes before letting others read. This will have reads be
-                        // 100% sequential.
                         PageCursorUtil.goTo( cursor, "clean", id );
 
                         if ( hasCrashedGSPP( treeNode, cursor ) )
@@ -171,6 +166,7 @@ private boolean hasCrashedGSPP( TreeNode<?,?> treeNode, PageCursor cursor ) thro
             keyCount = treeNode.keyCount( cursor );
         }
         while ( cursor.shouldRetry() );
+        PageCursorUtil.checkOutOfBounds( cursor );
 
         if ( !isTreeNode )
         {
@@ -187,13 +183,14 @@ private boolean hasCrashedGSPP( TreeNode<?,?> treeNode, PageCursor cursor ) thro
 
             if ( !hasCrashed && TreeNode.isInternal( cursor ) )
             {
-                for ( int i = 0; i <= keyCount && i <= maxKeyCount && !hasCrashed; i++ )
+                for ( int i = 0; i <= keyCount && i <= internalMaxKeyCount && !hasCrashed; i++ )
                 {
-                    hasCrashed |= hasCrashedGSPP( cursor, treeNode.childOffset( i ) );
+                    hasCrashed = hasCrashedGSPP( cursor, treeNode.childOffset( i ) );
                 }
             }
         }
         while ( cursor.shouldRetry() );
+        PageCursorUtil.checkOutOfBounds( cursor );
         return hasCrashed;
     }
 
@@ -220,7 +217,7 @@ private void cleanTreeNode( TreeNode<?,?> treeNode, PageCursor cursor, AtomicInt
         if ( TreeNode.isInternal( cursor ) )
         {
             int keyCount = treeNode.keyCount( cursor );
-            for ( int i = 0; i <= keyCount && i <= maxKeyCount; i++ )
+            for ( int i = 0; i <= keyCount && i <= internalMaxKeyCount; i++ )
             {
                 cleanCrashedGSPP( cursor, treeNode.childOffset( i ), cleanedPointers );
             }

diff --git a/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTree.java b/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTree.java
@@ -848,7 +848,7 @@ public void prepareForRecovery() throws IOException
      *
      * @throws IOException on {@link PageCache} error.
      */
-    public void completeRecovery() throws IOException
+    public void cleanCrashPointers() throws IOException
     {
         // For the time being there's an issue where update that come in before a crash
         // may be applied in a different order than when they get recovered.

diff --git a/community/index/src/test/java/org/neo4j/index/internal/gbptree/CrashGenCleanerTest.java b/community/index/src/test/java/org/neo4j/index/internal/gbptree/CrashGenCleanerTest.java
@@ -28,7 +28,6 @@
 import org.junit.rules.RuleChain;
 
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
@@ -70,19 +69,19 @@ public class CrashGenCleanerTest
     private final int firstChildPos = 0;
     private final int middleChildPos = corruptableTreeNode.internalMaxKeyCount() / 2;
     private final int lastChildPos = corruptableTreeNode.internalMaxKeyCount();
-    private final List<PageCorruption> possibleCorruptionsInInternal = new ArrayList<>( Arrays.asList(
+    private final List<PageCorruption> possibleCorruptionsInInternal = Arrays.asList(
             crashed( leftSibling() ),
             crashed( rightSibling() ),
             crashed( newGen() ),
             crashed( child( firstChildPos ) ),
             crashed( child( middleChildPos ) ),
             crashed( child( lastChildPos ) )
-    ) );
-    private final List<PageCorruption> possibleCorruptionsInLeaf = new ArrayList<>( Arrays.asList(
+    );
+    private final List<PageCorruption> possibleCorruptionsInLeaf = Arrays.asList(
             crashed( leftSibling() ),
             crashed( rightSibling() ),
             crashed( newGen() )
-    ) );
+    );
 
     private class SimpleMonitor implements GBPTree.Monitor
     {

diff --git a/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeRecoveryTest.java b/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeRecoveryTest.java
@@ -100,7 +100,7 @@ public void shouldRecoverFromCrashBeforeFirstCheckpoint() throws Exception
         {
             // this is the mimic:ed recovery
             index.prepareForRecovery();
-            index.completeRecovery();
+            index.cleanCrashPointers();
 
             try ( Writer<MutableLong,MutableLong> writer = index.writer() )
             {
@@ -231,7 +231,7 @@ private void doShouldRecoverFromAnything( boolean replayRecoveryExactlyFromCheck
                 GBPTree<MutableLong,MutableLong> index = createIndex( pageCache, file ) )
         {
             recover( recoveryActions, index );
-            index.completeRecovery();
+            index.cleanCrashPointers();
 
             // THEN
             // we should end up with a consistent index containing all the stuff load says

diff --git a/community/io/src/test/java/org/neo4j/adversaries/RandomAdversary.java b/community/io/src/test/java/org/neo4j/adversaries/RandomAdversary.java
@@ -40,7 +40,7 @@ public RandomAdversary( double mischiefRate, double failureRate, double errorRat
         assert 0 <= errorRate && errorRate < 1.0 :
                 "Expected error rate in [0.0; 1.0[ but was " + errorRate;
         assert mischiefRate + errorRate + failureRate < 1.0 :
-                "Expected error rate + failure rate in [0.0; 1.0[ but was " +
+                "Expected mischief rate + error rate + failure rate in [0.0; 1.0[ but was " +
                         (mischiefRate + errorRate + failureRate);
 
         this.mischiefRate = mischiefRate;

diff --git a/...nity/kernel/src/main/java/org/neo4j/kernel/impl/index/labelscan/NativeLabelScanStore.java b/...nity/kernel/src/main/java/org/neo4j/kernel/impl/index/labelscan/NativeLabelScanStore.java
@@ -391,7 +391,7 @@ private void maybeCompleteRecovery() throws IOException
     {
         if ( recoveryStarted )
         {
-            index.completeRecovery();
+            index.cleanCrashPointers();
             recoveryStarted = false;
         }
     }