Various cleanups as result of removing StoreType from StoreFileMetadata

- Remove StoreType from StoreFileMetadata - Better testing around store file listing, including explicit tests for StoreType.META_DATA being listed last. - Added tests for NeoStores.isPresent()
neo4j · May 17, 2017 · 319302c · 319302c
1 parent 77414ae
commit 319302c
Show file tree

Hide file tree

Showing 10 changed files with 171 additions and 167 deletions.
diff --git a/...main/java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageEngine.java b/...main/java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageEngine.java
@@ -24,7 +24,6 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
-import java.util.Optional;
 import java.util.function.Supplier;
 
 import org.neo4j.concurrent.WorkSync;
@@ -524,8 +523,7 @@ public Collection<StoreFileMetadata> listStorageFiles()
             {
                 final RecordStore<AbstractBaseRecord> recordStore = neoStores.getRecordStore( type );
                 StoreFileMetadata metadata =
-                        new StoreFileMetadata( recordStore.getStorageFileName(), Optional.of( type ),
-                                recordStore.getRecordSize() );
+                        new StoreFileMetadata( recordStore.getStorageFileName(), recordStore.getRecordSize() );
                 files.add( metadata );
             }
         }
@@ -538,7 +536,7 @@ private void addCountStoreFiles( List<StoreFileMetadata> files )
         for ( File countStoreFile : countStoreFiles )
         {
             StoreFileMetadata countStoreFileMetadata = new StoreFileMetadata( countStoreFile,
-                    Optional.of( StoreType.COUNTS ), RecordFormat.NO_RECORD_SIZE );
+                    RecordFormat.NO_RECORD_SIZE );
             files.add( countStoreFileMetadata );
         }
     }

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/StoreType.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/StoreType.java
@@ -174,6 +174,13 @@ public String getStoreName()
                 {
                     return StoreFactory.COUNTS_STORE;
                 }
+
+                @Override
+                protected boolean isStoreFile( String fileName)
+                {
+                    return matchStoreName( fileName, getStoreName() + CountsTracker.RIGHT ) ||
+                           matchStoreName( fileName, getStoreName() + CountsTracker.LEFT );
+                }
             },
     META_DATA( StoreFile.NEO_STORE ) // Make sure this META store is last
             {
@@ -232,7 +239,7 @@ public static Optional<StoreType> typeOf( String fileName )
         StoreType[] values = StoreType.values();
         for ( StoreType value : values )
         {
-            if ( fileName.equals( MetaDataStore.DEFAULT_NAME + value.getStoreName() ) )
+            if ( value.isStoreFile( fileName ) )
             {
                 return Optional.of( value );
             }
@@ -252,4 +259,14 @@ public static boolean shouldBeManagedByPageCache( String storeFileName )
         boolean isLabelScanStore = NativeLabelScanStore.FILE_NAME.equals( storeFileName );
         return isLabelScanStore || StoreType.typeOf( storeFileName ).map( StoreType::isRecordStore ).orElse( false );
     }
+
+    protected boolean isStoreFile( String fileName )
+    {
+        return matchStoreName( fileName, getStoreName() );
+    }
+
+    protected boolean matchStoreName( String fileName, String storeName )
+    {
+        return fileName.equals( MetaDataStore.DEFAULT_NAME + storeName );
+    }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/LogFiles.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/LogFiles.java
@@ -33,7 +33,7 @@ public class LogFiles
 {
     public static final FilenameFilter FILENAME_FILTER = new LogicalLogFilenameFilter();
 
-    public static final class LogicalLogFilenameFilter implements FilenameFilter
+    private static final class LogicalLogFilenameFilter implements FilenameFilter
     {
         private static final Pattern LOG_FILENAME_PATTERN = compile(
                 PhysicalLogFile.REGEX_DEFAULT_NAME + PhysicalLogFile.REGEX_DEFAULT_VERSION_SUFFIX + ".*"

diff --git a/...ity/kernel/src/main/java/org/neo4j/kernel/impl/transaction/state/NeoStoreFileListing.java b/...ity/kernel/src/main/java/org/neo4j/kernel/impl/transaction/state/NeoStoreFileListing.java
@@ -23,6 +23,7 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.List;
 import java.util.Optional;
 import java.util.function.Function;
 import java.util.stream.Collectors;
@@ -34,6 +35,7 @@
 import org.neo4j.kernel.impl.api.index.IndexingService;
 import org.neo4j.kernel.impl.index.IndexConfigStore;
 import org.neo4j.kernel.impl.store.MetaDataStore;
+import org.neo4j.kernel.impl.store.StoreType;
 import org.neo4j.kernel.impl.store.format.RecordFormat;
 import org.neo4j.kernel.spi.legacyindex.IndexImplementation;
 import org.neo4j.storageengine.api.StorageEngine;
@@ -50,7 +52,7 @@ public class NeoStoreFileListing
     private final LegacyIndexProviderLookup legacyIndexProviders;
     private final StorageEngine storageEngine;
     private final Function<File,StoreFileMetadata> toNotAStoreTypeFile =
-            file -> new StoreFileMetadata( file, Optional.empty(), RecordFormat.NO_RECORD_SIZE );
+            file -> new StoreFileMetadata( file, RecordFormat.NO_RECORD_SIZE );
 
     public NeoStoreFileListing( File storeDir, LabelScanStore labelScanStore, IndexingService indexingService,
             LegacyIndexProviderLookup legacyIndexProviders, StorageEngine storageEngine )
@@ -64,17 +66,38 @@ public NeoStoreFileListing( File storeDir, LabelScanStore labelScanStore, Indexi
 
     public ResourceIterator<StoreFileMetadata> listStoreFiles( boolean includeLogs ) throws IOException
     {
-        Collection<StoreFileMetadata> files = new ArrayList<>();
+        List<StoreFileMetadata> files = new ArrayList<>();
         gatherNonRecordStores( files, includeLogs );
         gatherNeoStoreFiles( files );
         Resource labelScanStoreSnapshot = gatherLabelScanStoreFiles( files );
         Resource schemaIndexSnapshots = gatherSchemaIndexFiles( files );
         Resource legacyIndexSnapshots = gatherLegacyIndexFiles( files );
 
+        placeMetaDataStoreLast( files );
+
         return resourceIterator( files.iterator(),
                 new MultiResource( asList( labelScanStoreSnapshot, schemaIndexSnapshots, legacyIndexSnapshots ) ) );
     }
 
+    private void placeMetaDataStoreLast( List<StoreFileMetadata> files )
+    {
+        int index = 0;
+        for ( StoreFileMetadata file : files )
+        {
+            Optional<StoreType> storeType = StoreType.typeOf( file.file().getName() );
+            if ( storeType.isPresent() && storeType.get().equals( StoreType.META_DATA ) )
+            {
+                break;
+            }
+            index++;
+        }
+        if ( index < files.size() - 1 )
+        {
+            StoreFileMetadata metaDataStoreFile = files.remove( index );
+            files.add( metaDataStoreFile );
+        }
+    }
+
     private void gatherNonRecordStores( Collection<StoreFileMetadata> files, boolean includeLogs )
     {
         final File[] storeFiles = storeDir.listFiles();

diff --git a/community/kernel/src/main/java/org/neo4j/storageengine/api/StoreFileMetadata.java b/community/kernel/src/main/java/org/neo4j/storageengine/api/StoreFileMetadata.java
@@ -20,20 +20,15 @@
 package org.neo4j.storageengine.api;
 
 import java.io.File;
-import java.util.Optional;
-
-import org.neo4j.kernel.impl.store.StoreType;
 
 public class StoreFileMetadata
 {
     private final File file;
-    private final Optional<StoreType> storeType;
     private final int recordSize;
 
-    public StoreFileMetadata( File file, Optional<StoreType> storeType, int recordSize )
+    public StoreFileMetadata( File file, int recordSize )
     {
         this.file = file;
-        this.storeType = storeType;
         this.recordSize = recordSize;
     }
 
@@ -42,11 +37,6 @@ public File file()
         return file;
     }
 
-    public Optional<StoreType> storeType()
-    {
-        return storeType;
-    }
-
     public int recordSize()
     {
         return recordSize;

diff --git a/.../java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageEngineTest.java b/.../java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageEngineTest.java
@@ -25,10 +25,14 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.ThreadLocalRandom;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Function;
+import java.util.stream.Collectors;
 
 import org.neo4j.helpers.Exceptions;
 import org.neo4j.io.fs.FileSystemAbstraction;
@@ -39,12 +43,9 @@
 import org.neo4j.kernel.impl.api.BatchTransactionApplierFacade;
 import org.neo4j.kernel.impl.api.CountsAccessor;
 import org.neo4j.kernel.impl.api.TransactionToApply;
-import org.neo4j.kernel.impl.store.StoreFile;
 import org.neo4j.kernel.impl.store.StoreType;
 import org.neo4j.kernel.impl.store.UnderlyingStorageException;
 import org.neo4j.kernel.impl.store.counts.CountsTracker;
-import org.neo4j.kernel.impl.store.format.RecordFormat;
-import org.neo4j.kernel.impl.storemigration.StoreFileType;
 import org.neo4j.kernel.impl.transaction.TransactionRepresentation;
 import org.neo4j.kernel.impl.transaction.log.FakeCommitment;
 import org.neo4j.kernel.impl.transaction.log.TransactionIdStore;
@@ -56,13 +57,11 @@
 import org.neo4j.test.rule.RecordStorageEngineRule;
 import org.neo4j.test.rule.fs.EphemeralFileSystemRule;
 
-import static org.hamcrest.Matchers.anyOf;
-import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.sameInstance;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertSame;
 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.doThrow;
@@ -83,6 +82,12 @@ public class RecordStorageEngineTest
             .around( pageCacheRule )
             .around( storageEngineRule );
 
+    private static final Function<Optional<StoreType>,StoreType> assertIsPresentAndGet = optional ->
+    {
+        assert optional.isPresent() : "Expected optional to be present";
+        return optional.get();
+    };
+
     @Test( timeout = 30_000 )
     public void shutdownRecordStorageEngineAfterFailedTransaction() throws Throwable
     {
@@ -167,42 +172,19 @@ public void flushAndForce( IOLimiter limiter ) throws IOException
     }
 
     @Test
-    public void shouldListAllFiles() throws Throwable
+    public void shouldListAllStoreFiles() throws Throwable
     {
         RecordStorageEngine engine = buildRecordStorageEngine();
 
         final Collection<StoreFileMetadata> files = engine.listStorageFiles();
-        assertTrue( files.size() > 0 );
-        files.forEach( this::verifyMeta );
-    }
+        Set<StoreType> expectedStoreTypes = Arrays.stream( StoreType.values() ).collect( Collectors.toSet() );
 
-    private void verifyMeta( StoreFileMetadata meta )
-    {
-        final Optional<StoreType> optional = meta.storeType();
-        if ( optional.isPresent() )
-        {
-            final StoreType type = optional.get();
-            final File file = meta.file();
-            final String fileName = file.getName();
-            if ( type == StoreType.COUNTS )
-            {
-                final String left = StoreFile.COUNTS_STORE_LEFT.fileName( StoreFileType.STORE );
-                final String right = StoreFile.COUNTS_STORE_RIGHT.fileName( StoreFileType.STORE );
-                assertThat( fileName, anyOf( equalTo( left ), equalTo( right ) ) );
-            }
-            else
-            {
-                final String expected = type.getStoreFile().fileName( StoreFileType.STORE );
-                assertThat( fileName, equalTo( expected ) );
-                assertTrue( "File does not exist " + file.getAbsolutePath(), fsRule.get().fileExists( file ) );
-            }
-            final int recordSize = meta.recordSize();
-            assertTrue( recordSize == RecordFormat.NO_RECORD_SIZE || recordSize > 0 );
-        }
-        else
-        {
-            fail( "Assumed all files to have a store type" );
-        }
+        Set<StoreType> actualStoreTypes = files.stream()
+                .map( storeFileMetadata -> StoreType.typeOf( storeFileMetadata.file().getName() ) )
+                .map( assertIsPresentAndGet )
+                .collect( Collectors.toSet() );
+
+        assertEquals( expectedStoreTypes, actualStoreTypes );
     }
 
     private RecordStorageEngine buildRecordStorageEngine() throws Throwable