Unmap page cache if count store rotation times out

It also makes ProgressiveState state Closeable.
neo4j · Nov 14, 2016 · 692ffd7 · 692ffd7
1 parent 619ab47
commit 692ffd7
Show file tree

Hide file tree

Showing 9 changed files with 62 additions and 29 deletions.
diff --git a/...unity/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/AbstractKeyValueStore.java b/...unity/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/AbstractKeyValueStore.java
@@ -298,18 +298,17 @@ public void run()
 
         private long rotate( boolean force ) throws IOException
         {
-            final long version = rotation.rotationVersion();
-            ProgressiveState<Key> next = rotation.rotate( force, rotationStrategy, rotationTimerFactory,
-                    value -> updateHeaders( value, version ) );
-            try ( LockWrapper ignored = writeLock( updateLock ) )
-            {
-                state = next;
-            }
-            finally
+            try( RotationState<Key> rotation = this.rotation )
             {
-                rotation.close();
+                final long version = rotation.rotationVersion();
+                ProgressiveState<Key> next = rotation.rotate( force, rotationStrategy, rotationTimerFactory,
+                        value -> updateHeaders( value, version ) );
+                try ( LockWrapper ignored = writeLock( updateLock ) )
+                {
+                    state = next;
+                }
+                return version;
             }
-            return version;
         }
     }
 

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/ActiveState.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/ActiveState.java
@@ -103,9 +103,6 @@ protected final int storedEntryCount()
 
     protected abstract PrototypeState<Key> prototype( long version );
 
-    @Override
-    protected abstract void close() throws IOException;
-
     protected abstract Factory factory();
 
     protected abstract long applied();

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/ConcurrentMapState.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/ConcurrentMapState.java
@@ -319,7 +319,7 @@ protected boolean hasChanges()
     }
 
     @Override
-    protected void close() throws IOException
+    public void close() throws IOException
     {
         store.close();
     }

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/DeadState.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/DeadState.java
@@ -74,7 +74,7 @@ protected boolean hasChanges()
     }
 
     @Override
-    void close() throws IOException
+    public void close() throws IOException
     {
         throw new IllegalStateException( "Cannot close() in state: " + stateName() );
     }
@@ -219,7 +219,7 @@ ProgressiveState<Key> rotate( boolean force, RotationStrategy strategy, Rotation
                 }
 
                 @Override
-                void close() throws IOException
+                public void close() throws IOException
                 {
                 }
 
@@ -282,7 +282,7 @@ ProgressiveState<Key> rotate( boolean force, RotationStrategy strategy, Rotation
                 }
 
                 @Override
-                void close() throws IOException
+                public void close() throws IOException
                 {
                     state.close();
                 }

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/ProgressiveState.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/ProgressiveState.java
@@ -67,7 +67,7 @@ EntryUpdater<Key> resetter( Lock lock, Runnable closeAction )
     }
 
     @Override
-    void close() throws IOException
+    public void close() throws IOException
     {
         throw new IllegalStateException( "Cannot close() in state: " + stateName() );
     }

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/PrototypeState.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/PrototypeState.java
@@ -60,7 +60,7 @@ final EntryUpdater<Key> resetter( Lock lock, Runnable runnable )
     }
 
     @Override
-    final void close() throws IOException
+    public final void close() throws IOException
     {
         throw new UnsupportedOperationException( "should never be invoked" );
     }

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/ReadableState.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/ReadableState.java
@@ -19,11 +19,14 @@
  */
 package org.neo4j.kernel.impl.store.kvstore;
 
+import java.io.Closeable;
 import java.io.IOException;
 
+import org.neo4j.io.pagecache.tracing.AutoCloseablePageCacheTracerEvent;
+
 import static org.neo4j.kernel.impl.store.kvstore.DataProvider.EMPTY_DATA_PROVIDER;
 
-abstract class ReadableState<Key>
+abstract class ReadableState<Key> implements Closeable
 {
     protected abstract KeyFormat<Key> keyFormat();
 
@@ -37,8 +40,6 @@ abstract class ReadableState<Key>
 
     protected abstract int storedEntryCount();
 
-    abstract void close() throws IOException;
-
     static <Key> ReadableState<Key> store( final KeyFormat<Key> keys, final KeyValueStoreFile store )
     {
         return new ReadableState<Key>()
@@ -80,7 +81,7 @@ protected int storedEntryCount()
             }
 
             @Override
-            void close() throws IOException
+            public void close() throws IOException
             {
                 store.close();
             }
@@ -128,7 +129,7 @@ protected int storedEntryCount()
             }
 
             @Override
-            void close() throws IOException
+            public void close() throws IOException
             {
             }
         };

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/RotationState.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/kvstore/RotationState.java
@@ -40,9 +40,6 @@ String stateName()
         return "rotating";
     }
 
-    @Override
-    abstract void close() throws IOException;
-
     abstract long rotationVersion();
 
     static final class Rotation<Key> extends RotationState<Key>
@@ -88,7 +85,7 @@ ActiveState<Key> rotate( boolean force, RotationStrategy strategy, RotationTimer
         }
 
         @Override
-        void close() throws IOException
+        public void close() throws IOException
         {
             preState.close();
         }

diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/counts/CountsRotationTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/counts/CountsRotationTest.java
@@ -27,6 +27,7 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ForkJoinPool;
@@ -44,10 +45,12 @@
 import org.neo4j.graphdb.Transaction;
 import org.neo4j.graphdb.TransactionFailureException;
 import org.neo4j.graphdb.factory.GraphDatabaseBuilder;
+import org.neo4j.graphdb.factory.GraphDatabaseSettings;
 import org.neo4j.helpers.collection.Pair;
 import org.neo4j.io.fs.FileSystemAbstraction;
 import org.neo4j.io.pagecache.PageCache;
 import org.neo4j.kernel.configuration.Config;
+import org.neo4j.kernel.impl.api.CountsAccessor;
 import org.neo4j.kernel.impl.api.CountsVisitor;
 import org.neo4j.kernel.impl.core.LabelTokenHolder;
 import org.neo4j.kernel.impl.storageengine.impl.recordstorage.RecordStorageEngine;
@@ -56,6 +59,7 @@
 import org.neo4j.kernel.impl.store.StoreFactory;
 import org.neo4j.kernel.impl.store.counts.keys.CountsKey;
 import org.neo4j.kernel.impl.store.counts.keys.CountsKeyFactory;
+import org.neo4j.kernel.impl.store.kvstore.RotationTimeoutException;
 import org.neo4j.kernel.impl.transaction.log.checkpoint.CheckPointer;
 import org.neo4j.kernel.impl.transaction.log.checkpoint.SimpleTriggerInfo;
 import org.neo4j.kernel.impl.transaction.log.checkpoint.TriggerInfo;
@@ -146,6 +150,36 @@ public void shouldCreateEmptyCountsTrackerStoreWhenCreatingDatabase() throws IOE
         }
     }
 
+    @Test
+    public void shouldUnMapThePrestateFileWhenTimingOutOnRotation() throws IOException
+    {
+        // Given
+        dbBuilder.newGraphDatabase().shutdown();
+        CountsTracker store = createCountsTracker( pageCache, Config.defaults().augment( Collections
+                .singletonMap( GraphDatabaseSettings.counts_store_rotation_timeout.name(), "100ms" ) ) );
+        store.init();
+        store.start();
+
+        try ( CountsAccessor.Updater updater = store.apply( 2 ).get() )
+        {
+            updater.incrementNodeCount( 0, 1 );
+        }
+
+        try
+        {
+            // when
+            store.rotate( 3 );
+            fail( "should have thrown" );
+        }
+        catch ( RotationTimeoutException ex )
+        {
+            // good
+        }
+
+        // then no exceptions closing the page cache
+        pageCache.close();
+    }
+
     @Test
     public void rotationShouldNotCauseUnmappedFileProblem() throws IOException
     {
@@ -343,7 +377,12 @@ private static void await( CountDownLatch latch )
 
     private CountsTracker createCountsTracker( PageCache pageCache )
     {
-        return new CountsTracker( NullLogProvider.getInstance(), fs, pageCache, Config.empty(),
+        return createCountsTracker( pageCache, Config.defaults() );
+    }
+
+    private CountsTracker createCountsTracker( PageCache pageCache, Config config )
+    {
+        return new CountsTracker( NullLogProvider.getInstance(), fs, pageCache, config,
                 new File( dir.getPath(), COUNTS_STORE_BASE ) );
     }