From 57ed5b0cdd0d764f2853db18e0f4d165a7cf5e49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20Finn=C3=A9?= Date: Fri, 1 Sep 2017 19:02:42 +0200 Subject: [PATCH] Addressed PR comments --- .../java/org/neo4j/test/rule/RepeatRule.java | 10 ++++--- .../kernel/api/index/SchemaIndexProvider.java | 10 +++---- .../recordstorage/RecordStorageEngine.java | 10 +++---- .../command/IndexBatchTransactionApplier.java | 2 -- .../command/NeoStoreTransactionApplier.java | 24 +++++++++------ .../ReversedSingleFileTransactionCursor.java | 2 +- .../kernel/recovery/DefaultRecoverySPI.java | 4 +-- .../org/neo4j/kernel/recovery/Recovery.java | 8 ++--- .../UpdateCapturingIndexProvider.java | 2 +- .../log/GivenTransactionCursor.java | 2 +- ...eversedMultiFileTransactionCursorTest.java | 30 ++++++++----------- .../neo4j/test/TestGraphDatabaseFactory.java | 4 +-- ...queDatabaseIndexPopulatingUpdaterTest.java | 15 ++++------ 13 files changed, 58 insertions(+), 65 deletions(-) diff --git a/community/common/src/test/java/org/neo4j/test/rule/RepeatRule.java b/community/common/src/test/java/org/neo4j/test/rule/RepeatRule.java index 2c2fcb645cb0f..1d0cd7430c732 100644 --- a/community/common/src/test/java/org/neo4j/test/rule/RepeatRule.java +++ b/community/common/src/test/java/org/neo4j/test/rule/RepeatRule.java @@ -28,8 +28,6 @@ import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; -import static java.lang.Integer.max; - /** * Set a test to loop a number of times. If you find yourself using this in a production test, you are probably doing * something wrong. @@ -95,9 +93,13 @@ public void evaluate() throws Throwable public Statement apply( Statement base, Description description ) { Repeat repeat = description.getAnnotation( Repeat.class ); - if ( repeat != null || defaultTimes > 1 ) + if ( repeat != null ) + { + return new RepeatStatement( repeat.times(), base, description ); + } + if ( defaultTimes > 1 ) { - return new RepeatStatement( max( repeat != null ? repeat.times() : 1, defaultTimes ), base, description ); + return new RepeatStatement( defaultTimes, base, description ); } return base; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/index/SchemaIndexProvider.java b/community/kernel/src/main/java/org/neo4j/kernel/api/index/SchemaIndexProvider.java index d57d76eeabdd1..241d9504a98c1 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/index/SchemaIndexProvider.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/index/SchemaIndexProvider.java @@ -135,6 +135,11 @@ public String getPopulationFailure( long indexId ) throws IllegalStateException protected final int priority; private final Descriptor providerDescriptor; + protected SchemaIndexProvider( SchemaIndexProvider copySource ) + { + this( copySource.providerDescriptor, copySource.priority ); + } + protected SchemaIndexProvider( Descriptor descriptor, int priority ) { assert descriptor != null; @@ -142,11 +147,6 @@ protected SchemaIndexProvider( Descriptor descriptor, int priority ) this.providerDescriptor = descriptor; } - public int priority() - { - return this.priority; - } - /** * Used for initially populating a created index, using batch insertion. */ diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageEngine.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageEngine.java index 0599541d8c88b..dc79e1685202c 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageEngine.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageEngine.java @@ -125,6 +125,8 @@ import org.neo4j.unsafe.impl.internal.dragons.FeatureToggles; import static org.neo4j.kernel.impl.locking.LockService.NO_LOCK_SERVICE; +import static org.neo4j.storageengine.api.TransactionApplicationMode.RECOVERY; +import static org.neo4j.storageengine.api.TransactionApplicationMode.REVERSE_RECOVERY; public class RecordStorageEngine implements StorageEngine, Lifecycle { @@ -165,7 +167,6 @@ public class RecordStorageEngine implements StorageEngine, Lifecycle private final RelationshipDeleter relationshipDeleter; private final PropertyCreator propertyCreator; private final PropertyDeleter propertyDeleter; - private boolean started; public RecordStorageEngine( File storeDir, @@ -358,7 +359,7 @@ protected BatchTransactionApplierFacade applier( TransactionApplicationMode mode { ArrayList appliers = new ArrayList<>(); // Graph store application. The order of the decorated store appliers is irrelevant - appliers.add( new NeoStoreBatchTransactionApplier( mode.version(), neoStores, cacheAccess, lockService() ) ); + appliers.add( new NeoStoreBatchTransactionApplier( mode.version(), neoStores, cacheAccess, lockService( mode ) ) ); if ( mode.needsHighIdTracking() ) { appliers.add( new HighIdBatchTransactionApplier( neoStores ) ); @@ -388,9 +389,9 @@ protected BatchTransactionApplierFacade applier( TransactionApplicationMode mode appliers.toArray( new BatchTransactionApplier[appliers.size()] ) ); } - private LockService lockService() + private LockService lockService( TransactionApplicationMode mode ) { - return started ? lockService : NO_LOCK_SERVICE; + return mode == RECOVERY || mode == REVERSE_RECOVERY ? NO_LOCK_SERVICE : lockService; } public void satisfyDependencies( DependencySatisfier satisfier ) @@ -430,7 +431,6 @@ public void start() throws Throwable indexingService.start(); labelScanStore.start(); idController.start(); - started = true; } @Override diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/IndexBatchTransactionApplier.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/IndexBatchTransactionApplier.java index 5908d1f928f19..341bfbd0831ff 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/IndexBatchTransactionApplier.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/IndexBatchTransactionApplier.java @@ -62,7 +62,6 @@ public class IndexBatchTransactionApplier extends BatchTransactionApplier.Adapte private List labelUpdates; private IndexUpdates indexUpdates; - private long txId; public IndexBatchTransactionApplier( IndexingService indexingService, WorkSync,LabelUpdateWork> labelScanStoreSync, @@ -80,7 +79,6 @@ public IndexBatchTransactionApplier( IndexingService indexingService, @Override public TransactionApplier startTx( CommandsToApply transaction ) { - txId = transaction.transactionId(); return transactionApplier; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/NeoStoreTransactionApplier.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/NeoStoreTransactionApplier.java index 79dea6992fe8c..db92e15341e8e 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/NeoStoreTransactionApplier.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/NeoStoreTransactionApplier.java @@ -26,10 +26,12 @@ import org.neo4j.kernel.impl.locking.LockGroup; import org.neo4j.kernel.impl.locking.LockService; import org.neo4j.kernel.impl.store.NeoStores; +import org.neo4j.kernel.impl.store.RecordStore; import org.neo4j.kernel.impl.store.SchemaStore; +import org.neo4j.kernel.impl.store.record.AbstractBaseRecord; import org.neo4j.kernel.impl.store.record.ConstraintRule; import org.neo4j.kernel.impl.store.record.DynamicRecord; -import org.neo4j.kernel.impl.store.record.PropertyRecord; +import org.neo4j.kernel.impl.transaction.command.Command.BaseCommand; import org.neo4j.kernel.impl.transaction.command.Command.Version; /** @@ -73,7 +75,7 @@ public boolean visitNodeCommand( Command.NodeCommand command ) throws IOExceptio lockGroup.add( lockService.acquireNodeLock( command.getKey(), LockService.LockType.WRITE_LOCK ) ); // update store - neoStores.getNodeStore().updateRecord( version.select( command ) ); + updateStore( neoStores.getNodeStore(), command ); return false; } @@ -82,7 +84,7 @@ public boolean visitRelationshipCommand( Command.RelationshipCommand command ) t { lockGroup.add( lockService.acquireRelationshipLock( command.getKey(), LockService.LockType.WRITE_LOCK ) ); - neoStores.getRelationshipStore().updateRecord( version.select( command ) ); + updateStore( neoStores.getRelationshipStore(), command ); return false; } @@ -99,36 +101,35 @@ else if ( command.getRelId() != -1 ) lockGroup.add( lockService.acquireRelationshipLock( command.getRelId(), LockService.LockType.WRITE_LOCK ) ); } - PropertyRecord select = version.select( command ); - neoStores.getPropertyStore().updateRecord( select ); + updateStore( neoStores.getPropertyStore(), command ); return false; } @Override public boolean visitRelationshipGroupCommand( Command.RelationshipGroupCommand command ) throws IOException { - neoStores.getRelationshipGroupStore().updateRecord( version.select( command ) ); + updateStore( neoStores.getRelationshipGroupStore(), command ); return false; } @Override public boolean visitRelationshipTypeTokenCommand( Command.RelationshipTypeTokenCommand command ) throws IOException { - neoStores.getRelationshipTypeTokenStore().updateRecord( version.select( command ) ); + updateStore( neoStores.getRelationshipTypeTokenStore(), command ); return false; } @Override public boolean visitLabelTokenCommand( Command.LabelTokenCommand command ) throws IOException { - neoStores.getLabelTokenStore().updateRecord( version.select( command ) ); + updateStore( neoStores.getLabelTokenStore(), command ); return false; } @Override public boolean visitPropertyKeyTokenCommand( Command.PropertyKeyTokenCommand command ) throws IOException { - neoStores.getPropertyKeyTokenStore().updateRecord( version.select( command ) ); + updateStore( neoStores.getPropertyKeyTokenStore(), command ); return false; } @@ -183,4 +184,9 @@ public boolean visitNeoStoreCommand( Command.NeoStoreCommand command ) throws IO neoStores.getMetaDataStore().setGraphNextProp( version.select( command ).getNextProp() ); return false; } + + private void updateStore( RecordStore store, BaseCommand command ) + { + store.updateRecord( version.select( command ) ); + } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/log/ReversedSingleFileTransactionCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/log/ReversedSingleFileTransactionCursor.java index ae9b9a0663acf..9b862c64d08dc 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/log/ReversedSingleFileTransactionCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/log/ReversedSingleFileTransactionCursor.java @@ -97,7 +97,7 @@ private long[] sketchOutTransactionStartOffsets() throws IOException @Override public boolean next() throws IOException { - while ( !exhausted() ) + if ( !exhausted() ) { if ( currentChunkExhausted() ) { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/recovery/DefaultRecoverySPI.java b/community/kernel/src/main/java/org/neo4j/kernel/recovery/DefaultRecoverySPI.java index 849014316de15..0648558716f76 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/recovery/DefaultRecoverySPI.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/recovery/DefaultRecoverySPI.java @@ -48,7 +48,6 @@ public class DefaultRecoverySPI implements Recovery.SPI private final StorageEngine storageEngine; private final TransactionIdStore transactionIdStore; private final LogicalTransactionStore logicalTransactionStore; - private TransactionQueue transactionsToApply; public DefaultRecoverySPI( StorageEngine storageEngine, @@ -86,8 +85,7 @@ public void startRecovery() @Override public RecoveryApplier getRecoveryApplier( TransactionApplicationMode mode ) throws Exception { - transactionsToApply = new TransactionQueue( 100, ( first, last ) -> storageEngine.apply( first, mode ) ); - return new RecoveryVisitor( transactionsToApply ); + return new RecoveryVisitor( new TransactionQueue( 100, ( first, last ) -> storageEngine.apply( first, mode ) ) ); } @Override diff --git a/community/kernel/src/main/java/org/neo4j/kernel/recovery/Recovery.java b/community/kernel/src/main/java/org/neo4j/kernel/recovery/Recovery.java index c824c1e0e7505..960f5e49e7eb2 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/recovery/Recovery.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/recovery/Recovery.java @@ -52,7 +52,7 @@ default void recoveryCompleted( int numberOfRecoveredTransactions ) { // no-op by default } - default void reverseStoreRecoveryCompleted( long checkpointTxId ) + default void reverseStoreRecoveryCompleted( long lowestRecoveredTxId ) { // no-op by default } } @@ -102,18 +102,18 @@ public void init() throws Throwable spi.startRecovery(); // Backwards for neo store only - long checkpointTxId = TransactionIdStore.BASE_TX_ID; + long lowestRecoveredTxId = TransactionIdStore.BASE_TX_ID; try ( TransactionCursor transactionsToRecover = spi.getTransactionsInReverseOrder( recoveryFromPosition ); RecoveryApplier recoveryVisitor = spi.getRecoveryApplier( REVERSE_RECOVERY ); ) { while ( transactionsToRecover.next() ) { recoveryVisitor.visit( transactionsToRecover.get() ); - checkpointTxId = transactionsToRecover.get().getCommitEntry().getTxId(); + lowestRecoveredTxId = transactionsToRecover.get().getCommitEntry().getTxId(); } } - monitor.reverseStoreRecoveryCompleted( checkpointTxId ); + monitor.reverseStoreRecoveryCompleted( lowestRecoveredTxId ); // Forward with all appliers LogPosition recoveryToPosition; diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/index/inmemory/UpdateCapturingIndexProvider.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/index/inmemory/UpdateCapturingIndexProvider.java index 57160dea2084d..61931654eb124 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/index/inmemory/UpdateCapturingIndexProvider.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/index/inmemory/UpdateCapturingIndexProvider.java @@ -44,7 +44,7 @@ public class UpdateCapturingIndexProvider extends SchemaIndexProvider public UpdateCapturingIndexProvider( SchemaIndexProvider actual, Map> initialUpdates ) { - super( actual.getProviderDescriptor(), actual.priority() ); + super( actual ); this.actual = actual; this.initialUpdates = initialUpdates; } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/log/GivenTransactionCursor.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/log/GivenTransactionCursor.java index 89076235b2269..e53490803381f 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/log/GivenTransactionCursor.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/log/GivenTransactionCursor.java @@ -30,7 +30,7 @@ public class GivenTransactionCursor implements TransactionCursor private int index = -1; private final CommittedTransactionRepresentation[] transactions; - public GivenTransactionCursor( CommittedTransactionRepresentation... transactions ) + private GivenTransactionCursor( CommittedTransactionRepresentation... transactions ) { this.transactions = transactions; } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/log/ReversedMultiFileTransactionCursorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/log/ReversedMultiFileTransactionCursorTest.java index 9e9e509a38dd0..47c6f4850d54f 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/log/ReversedMultiFileTransactionCursorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/log/ReversedMultiFileTransactionCursorTest.java @@ -20,8 +20,6 @@ package org.neo4j.kernel.impl.transaction.log; import org.junit.Test; -import org.mockito.invocation.InvocationOnMock; -import org.mockito.stubbing.Answer; import java.io.IOException; import java.util.concurrent.atomic.AtomicLong; @@ -135,25 +133,21 @@ private ThrowingFunction log( int... logs[logVersion] = transactions( transactionCounts[logVersion], txId ); } - when( result.apply( any( LogPosition.class ) ) ).thenAnswer( new Answer() + when( result.apply( any( LogPosition.class ) ) ).thenAnswer( invocation -> { - @Override - public TransactionCursor answer( InvocationOnMock invocation ) throws Throwable + LogPosition position = invocation.getArgumentAt( 0, LogPosition.class ); + if ( position == null ) { - LogPosition position = invocation.getArgumentAt( 0, LogPosition.class ); - if ( position == null ) - { - // A mockito issue when calling the "when" methods, I believe - return null; - } - - // For simplicity the offset means, in this test, the array offset - CommittedTransactionRepresentation[] transactions = logs[toIntExact( position.getLogVersion() )]; - CommittedTransactionRepresentation[] subset = - copyOfRange( transactions, toIntExact( position.getByteOffset() - baseOffset ), transactions.length ); - ArrayUtil.reverse( subset ); - return given( subset ); + // A mockito issue when calling the "when" methods, I believe + return null; } + + // For simplicity the offset means, in this test, the array offset + CommittedTransactionRepresentation[] transactions = logs[toIntExact( position.getLogVersion() )]; + CommittedTransactionRepresentation[] subset = + copyOfRange( transactions, toIntExact( position.getByteOffset() - baseOffset ), transactions.length ); + ArrayUtil.reverse( subset ); + return given( subset ); } ); return result; } diff --git a/community/kernel/src/test/java/org/neo4j/test/TestGraphDatabaseFactory.java b/community/kernel/src/test/java/org/neo4j/test/TestGraphDatabaseFactory.java index 88fd510fa0422..96143e53ce854 100644 --- a/community/kernel/src/test/java/org/neo4j/test/TestGraphDatabaseFactory.java +++ b/community/kernel/src/test/java/org/neo4j/test/TestGraphDatabaseFactory.java @@ -237,12 +237,12 @@ public static class TestGraphDatabaseFacadeFactory extends GraphDatabaseFacadeFa private final TestGraphDatabaseFactoryState state; private final boolean impermanent; - public TestGraphDatabaseFacadeFactory( TestGraphDatabaseFactoryState state, boolean impermanent ) + protected TestGraphDatabaseFacadeFactory( TestGraphDatabaseFactoryState state, boolean impermanent ) { this( state, impermanent, DatabaseInfo.COMMUNITY, CommunityEditionModule::new ); } - public TestGraphDatabaseFacadeFactory( TestGraphDatabaseFactoryState state, boolean impermanent, + protected TestGraphDatabaseFacadeFactory( TestGraphDatabaseFactoryState state, boolean impermanent, DatabaseInfo databaseInfo, Function editionFactory ) { super( databaseInfo, editionFactory ); diff --git a/community/lucene-index/src/test/java/org/neo4j/kernel/api/impl/schema/populator/NonUniqueDatabaseIndexPopulatingUpdaterTest.java b/community/lucene-index/src/test/java/org/neo4j/kernel/api/impl/schema/populator/NonUniqueDatabaseIndexPopulatingUpdaterTest.java index 8050505563082..85117b9c1e8e4 100644 --- a/community/lucene-index/src/test/java/org/neo4j/kernel/api/impl/schema/populator/NonUniqueDatabaseIndexPopulatingUpdaterTest.java +++ b/community/lucene-index/src/test/java/org/neo4j/kernel/api/impl/schema/populator/NonUniqueDatabaseIndexPopulatingUpdaterTest.java @@ -201,9 +201,9 @@ public void additionsDeliveredToIndexWriter() throws Exception LuceneIndexWriter writer = mock( LuceneIndexWriter.class ); NonUniqueLuceneIndexPopulatingUpdater updater = newUpdater( writer ); - String expectedString1 = documentRepresentingProperties( (long) 1, "foo" ).toString(); - String expectedString2 = documentRepresentingProperties( (long) 2, "bar" ).toString(); - String expectedString3 = documentRepresentingProperties( (long) 3, "qux" ).toString(); + String expectedString1 = documentRepresentingProperties( 1, "foo" ).toString(); + String expectedString2 = documentRepresentingProperties( 2, "bar" ).toString(); + String expectedString3 = documentRepresentingProperties( 3, "qux" ).toString(); String expectedString4 = documentRepresentingProperties( 4, "git", "bit" ).toString(); updater.process( add( 1, SCHEMA_DESCRIPTOR, "foo" ) ); @@ -225,8 +225,8 @@ public void changesDeliveredToIndexWriter() throws Exception LuceneIndexWriter writer = mock( LuceneIndexWriter.class ); NonUniqueLuceneIndexPopulatingUpdater updater = newUpdater( writer ); - String expectedString1 = documentRepresentingProperties( (long) 1, "after1" ).toString(); - String expectedString2 = documentRepresentingProperties( (long) 2, "after2" ).toString(); + String expectedString1 = documentRepresentingProperties( 1, "after1" ).toString(); + String expectedString2 = documentRepresentingProperties( 2, "after2" ).toString(); String expectedString3 = documentRepresentingProperties( 3, "bit", "after2" ).toString(); updater.process( change( 1, SCHEMA_DESCRIPTOR, "before1", "after1" ) ); @@ -274,11 +274,6 @@ private static void verifySamplingResult( NonUniqueIndexSampler sampler, long ex assertEquals( expectedSampleSize, sample.sampleSize() ); } - private static NonUniqueLuceneIndexPopulatingUpdater newUpdater() - { - return newUpdater( newSampler() ); - } - private static NonUniqueLuceneIndexPopulatingUpdater newUpdater( NonUniqueIndexSampler sampler ) { return newUpdater( mock( LuceneIndexWriter.class ), sampler );