diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaIndexPopulator.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaIndexPopulator.java index ee132a8a7b9bb..9c4be9c75d495 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaIndexPopulator.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaIndexPopulator.java @@ -65,8 +65,9 @@ public abstract class NativeSchemaIndexPopulator singleWriter; private byte[] failureBytes; + private boolean dropped; - GBPTree gbpTree; + GBPTree tree; NativeSchemaIndexPopulator( PageCache pageCache, File storeFile, Layout layout, RecoveryCleanupWorkCollector recoveryCleanupWorkCollector ) @@ -81,26 +82,38 @@ public abstract class NativeSchemaIndexPopulator( pageCache, storeFile, layout, 0, NO_MONITOR, NO_HEADER, - recoveryCleanupWorkCollector ); + instantiateTree(); instantiateWriter(); } - protected void instantiateWriter() throws IOException + private void instantiateTree() throws IOException + { + tree = new GBPTree<>( pageCache, storeFile, layout, 0, NO_MONITOR, NO_HEADER, + recoveryCleanupWorkCollector ); + } + + void instantiateWriter() throws IOException { assert singleWriter == null; - singleWriter = gbpTree.writer(); + singleWriter = tree.writer(); } @Override - public void drop() throws IOException + public synchronized void drop() throws IOException { - closeWriter(); - gbpTree = closeIfPresent( gbpTree ); - GBPTreeUtil.deleteIfPresent( pageCache, storeFile ); + try + { + closeWriter(); + closeTree(); + GBPTreeUtil.deleteIfPresent( pageCache, storeFile ); + } + finally + { + dropped = true; + } } @Override @@ -141,24 +154,40 @@ public IndexUpdater newPopulatingUpdater( PropertyAccessor accessor ) throws IOE } @Override - public void close( boolean populationCompletedSuccessfully ) throws IOException + public synchronized void close( boolean populationCompletedSuccessfully ) throws IOException { - if ( populationCompletedSuccessfully && failureBytes != null ) + try { - throw new IllegalStateException( "Can't mark index as online after it has been marked as failure" ); + closeWriter(); + if ( populationCompletedSuccessfully && failureBytes != null ) + { + throw new IllegalStateException( "Can't mark index as online after it has been marked as failure" ); + } + if ( populationCompletedSuccessfully ) + { + assertPopulatorOpen(); + markTreeAsOnline(); + } + else + { + assertNotDropped(); + ensureTreeInstantiated(); + markTreeAsFailed(); + } } - closeWriter(); - if ( populationCompletedSuccessfully ) + finally { - markTreeAsOnline(); + closeTree(); } - else + } + + private void assertNotDropped() + { + if ( dropped ) { - markTreeAsFailed(); + throw new IllegalStateException( "Populator has already been dropped." ); } - gbpTree.close(); - gbpTree = null; -} + } @Override public void markAsFailed( String failure ) throws IOException @@ -166,18 +195,34 @@ public void markAsFailed( String failure ) throws IOException failureBytes = failure.getBytes( StandardCharsets.UTF_8 ); } + private void ensureTreeInstantiated() throws IOException + { + if ( tree == null ) + { + instantiateTree(); + } + } + + private void assertPopulatorOpen() + { + if ( tree == null ) + { + throw new IllegalStateException( "Populator has already been closed." ); + } + } + private void markTreeAsFailed() throws IOException { if ( failureBytes == null ) { failureBytes = "".getBytes(); } - gbpTree.checkpoint( IOLimiter.unlimited(), new FailureHeaderWriter( failureBytes ) ); + tree.checkpoint( IOLimiter.unlimited(), new FailureHeaderWriter( failureBytes ) ); } private void markTreeAsOnline() throws IOException { - gbpTree.checkpoint( IOLimiter.unlimited(), ( pc ) -> pc.putByte( BYTE_ONLINE ) ); + tree.checkpoint( IOLimiter.unlimited(), ( pc ) -> pc.putByte( BYTE_ONLINE ) ); } private T closeIfPresent( T closeable ) throws IOException @@ -189,11 +234,16 @@ private T closeIfPresent( T closeable ) throws IOException return null; } - protected void closeWriter() throws IOException + void closeWriter() throws IOException { singleWriter = closeIfPresent( singleWriter ); } + private void closeTree() throws IOException + { + tree = closeIfPresent( tree ); + } + private class NativeSchemaIndexUpdater implements IndexUpdater { private boolean closed; diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NonUniqueNativeSchemaIndexPopulator.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NonUniqueNativeSchemaIndexPopulator.java index 583946e40cbed..715b66d5e48da 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NonUniqueNativeSchemaIndexPopulator.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NonUniqueNativeSchemaIndexPopulator.java @@ -63,7 +63,7 @@ public void configureSampling( boolean onlineSampling ) { this.updateSampling = onlineSampling; this.sampler = onlineSampling ? new DefaultNonUniqueIndexSampler( samplingConfig.sampleSizeLimit() ) - : new FullScanNonUniqueIndexSampler<>( gbpTree, layout, samplingConfig ); + : new FullScanNonUniqueIndexSampler<>( tree, layout, samplingConfig ); } @Override diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaIndexPopulatorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaIndexPopulatorTest.java index beff4b6d244f6..35b2065b9cc9d 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaIndexPopulatorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaIndexPopulatorTest.java @@ -62,6 +62,7 @@ import org.neo4j.test.rule.RandomRule; import org.neo4j.test.rule.TestDirectory; import org.neo4j.test.rule.fs.DefaultFileSystemRule; +import org.neo4j.test.rule.fs.FileSystemRule; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -79,11 +80,11 @@ public abstract class NativeSchemaIndexPopulatorTest { - static final int LARGE_AMOUNT_OF_UPDATES = 10_000; + static final int LARGE_AMOUNT_OF_UPDATES = 1_000; private static final IndexDescriptor indexDescriptor = IndexDescriptorFactory.forLabel( 42, 666 ); static final PropertyAccessor null_property_accessor = ( nodeId, propKeyId ) -> null; - private final DefaultFileSystemRule fs = new DefaultFileSystemRule(); + private final FileSystemRule fs = new DefaultFileSystemRule(); private final TestDirectory directory = TestDirectory.testDirectory( getClass(), fs.get() ); private final PageCacheRule pageCacheRule = new PageCacheRule( config().withAccessChecks( true ) ); protected final RandomRule random = new RandomRule(); @@ -91,8 +92,8 @@ public abstract class NativeSchemaIndexPopulatorTest layout; - File indexFile; - PageCache pageCache; + private File indexFile; + private PageCache pageCache; NativeSchemaIndexPopulator populator; @Before @@ -114,13 +115,13 @@ abstract NativeSchemaIndexPopulator createPopulator( PageCache pageCa public void createShouldCreateFile() throws Exception { // given - assertFalse( fs.fileExists( indexFile ) ); + assertFileNotPresent(); // when populator.create(); // then - assertTrue( fs.fileExists( indexFile ) ); + assertFilePresent(); populator.close( true ); } @@ -154,17 +155,20 @@ public void dropShouldDeleteExistingFile() throws Exception populator.drop(); // then - assertFalse( fs.fileExists( indexFile ) ); + assertFileNotPresent(); } @Test public void dropShouldSucceedOnNonExistentFile() throws Exception { // given - assertFalse( fs.fileExists( indexFile ) ); + assertFileNotPresent(); - // then + // when populator.drop(); + + // then + assertFileNotPresent(); } @Test @@ -399,11 +403,154 @@ public void shouldApplyLargeAmountOfInterleavedRandomUpdates() throws Exception random.reset(); Random updaterRandom = new Random( random.seed() ); Iterator> updates = randomUniqueUpdateGenerator( random, 0 ); - int numberOfPopulatorUpdates = LARGE_AMOUNT_OF_UPDATES; // when + int count = interleaveLargeAmountOfUpdates( updaterRandom, updates ); + + // then + populator.close( true ); + random.reset(); + verifyUpdates( randomUniqueUpdateGenerator( random, 0 ), count ); + } + + @Test + public void dropMustSucceedAfterSuccessfulClose() throws Exception + { + // given + populator.create(); + populator.close( true ); + + // when + populator.drop(); + + // then + assertFileNotPresent(); + } + + @Test + public void dropMustSucceedAfterUnsuccessfulClose() throws Exception + { + // given + populator.create(); + populator.close( false ); + + // when + populator.drop(); + + // then + assertFileNotPresent(); + } + + @Test + public void successfulCloseMustThrowWithoutPriorSuccessfulCreate() throws Exception + { + // given + assertFileNotPresent(); + + // when + try + { + populator.close( true ); + fail( "Should have failed" ); + } + catch ( IllegalStateException e ) + { + // then good + } + } + + @Test + public void unsuccessfulCloseMustSucceedWithoutSuccessfulPriorCreate() throws Exception + { + // given + assertFileNotPresent(); + String failureMessage = "There is no spoon"; + populator.markAsFailed( failureMessage ); + + // when + populator.close( false ); + + // then + assertHeader( false, failureMessage, false ); + } + + @Test + public void successfulCloseMustThrowAfterDrop() throws Exception + { + // given + populator.create(); + + // when + populator.drop(); + + // then + try + { + populator.close( true ); + fail( "Should have failed" ); + } + catch ( IllegalStateException e ) + { + // then good + } + } + + @Test + public void unsuccessfulCloseMustThrowAfterDrop() throws Exception + { + // given + populator.create(); + + // when + populator.drop(); + + // then + try + { + populator.close( false ); + fail( "Should have failed" ); + } + catch ( IllegalStateException e ) + { + // then good + } + } + + private void assertFilePresent() + { + assertTrue( fs.fileExists( indexFile ) ); + } + + private void assertFileNotPresent() + { + assertFalse( fs.fileExists( indexFile ) ); + } + + static IndexEntryUpdate[] someIndexEntryUpdates() + { + return new IndexEntryUpdate[]{ + add( 0, 0 ), + add( 1, 4 ), + add( 2, Double.MAX_VALUE ), + add( 3, -Double.MAX_VALUE ), + add( 4, Float.MAX_VALUE ), + add( 5, -Float.MAX_VALUE ), + add( 6, Long.MAX_VALUE ), + add( 7, Long.MIN_VALUE ), + add( 8, Integer.MAX_VALUE ), + add( 9, Integer.MIN_VALUE ), + add( 10, Short.MAX_VALUE ), + add( 11, Short.MIN_VALUE ), + add( 12, Byte.MAX_VALUE ), + add( 13, Byte.MIN_VALUE ) + }; + } + + int interleaveLargeAmountOfUpdates( Random updaterRandom, + Iterator> updates ) throws IOException, IndexEntryConflictException + { int count = 0; - for ( int i = 0; i < numberOfPopulatorUpdates; i++ ) + for ( int i = 0; i < LARGE_AMOUNT_OF_UPDATES; i++ ) { if ( updaterRandom.nextFloat() < 0.1 ) { @@ -420,14 +567,10 @@ public void shouldApplyLargeAmountOfInterleavedRandomUpdates() throws Exception populator.add( updates.next() ); count++; } - - // then - populator.close( true ); - random.reset(); - verifyUpdates( randomUniqueUpdateGenerator( random, 0 ), count ); + return count; } - protected Iterator> randomUniqueUpdateGenerator( RandomRule randomRule, + Iterator> randomUniqueUpdateGenerator( RandomRule randomRule, float fractionDuplicates ) { return new PrefetchingIterator>() @@ -474,71 +617,6 @@ private Number existingNonUniqueValue( RandomRule randomRule ) }; } - @SuppressWarnings( "rawtypes" ) - // unique -> - // addShouldThrowOnDuplicateValues - // updaterShouldThrowOnDuplicateValues - // non-unique -> - // addShouldApplyDuplicateValues - // updaterShouldApplyDuplicateValues - - // successfulCloseMustCloseGBPTree - // successfulCloseMustCheckpointGBPTree (already verified by add / updater tests) - // successfulCloseMustMarkIndexAsOnline - - // unsuccessfulCloseMustCloseGBPTree - // unsuccessfulCloseMustNotMarkIndexAsOnline - // unsuccessfulCloseMustSucceedWithoutMarkAsFailed - - // closeMustWriteFailureMessageAfterMarkedAsFailed - // closeMustWriteFailureMessageAfterMarkedAsFailedWithLongMessage - // successfulCloseMustThrowIfMarkedAsFailed - - // shouldApplyLargeAmountOfInterleavedRandomUpdates - // unique -> - // shouldThrowOnLargeAmountOfInterleavedRandomUpdatesWithDuplicates - // non-unique -> - // shouldApplyLargeAmountOfInterleavedRandomUpdatesWithDuplicates - - // SAMPLING - // includeSample - // configureSampling - // sampleResult - - // ??? - // todo closeAfterDrop - // todo dropAfterClose - - // METHODS - // create() - // drop() - // add( Collection> updates ) - // add( IndexEntryUpdate update ) - // verifyDeferredConstraints( PropertyAccessor propertyAccessor ) - // newPopulatingUpdater( PropertyAccessor accessor ) - // close( boolean populationCompletedSuccessfully ) - // markAsFailed( String failure ) - - static IndexEntryUpdate[] someIndexEntryUpdates() - { - return new IndexEntryUpdate[]{ - add( 0, 0 ), - add( 1, 4 ), - add( 2, Double.MAX_VALUE ), - add( 3, -Double.MAX_VALUE ), - add( 4, Float.MAX_VALUE ), - add( 5, -Float.MAX_VALUE ), - add( 6, Long.MAX_VALUE ), - add( 7, Long.MIN_VALUE ), - add( 8, Integer.MAX_VALUE ), - add( 9, Integer.MIN_VALUE ), - add( 10, Short.MAX_VALUE ), - add( 11, Short.MIN_VALUE ), - add( 12, Byte.MAX_VALUE ), - add( 13, Byte.MIN_VALUE ) - }; - } - @SuppressWarnings( "rawtypes" ) static IndexEntryUpdate[] someDuplicateIndexEntryUpdates() { @@ -577,7 +655,7 @@ static IndexEntryUpdate[] someDuplicateIndexEntryUpdates() private void assertHeader( boolean online, String failureMessage, boolean messageTruncated ) throws IOException { NativeSchemaIndexHeaderReader headerReader = new NativeSchemaIndexHeaderReader(); - try ( GBPTree tree = new GBPTree<>( pageCache, indexFile, layout, 0, GBPTree.NO_MONITOR, + try ( GBPTree ignored = new GBPTree<>( pageCache, indexFile, layout, 0, GBPTree.NO_MONITOR, headerReader, RecoveryCleanupWorkCollector.IMMEDIATE ) ) { if ( online ) @@ -604,7 +682,7 @@ private void assertHeader( boolean online, String failureMessage, boolean messag private String longString( int length ) { String alphabet = "123xyz"; - StringBuffer outputBuffer = new StringBuffer( length ); + StringBuilder outputBuffer = new StringBuilder( length ); for ( int i = 0; i < length; i++ ) { outputBuffer.append( alphabet.charAt( random.nextInt( alphabet.length() ) ) ); @@ -612,7 +690,7 @@ private String longString( int length ) return outputBuffer.toString(); } - void applyInterleaved( IndexEntryUpdate[] updates, IndexUpdater updater, + private void applyInterleaved( IndexEntryUpdate[] updates, IndexUpdater updater, NativeSchemaIndexPopulator populator ) throws IOException, IndexEntryConflictException { for ( IndexEntryUpdate update : updates ) @@ -628,7 +706,7 @@ void applyInterleaved( IndexEntryUpdate[] updates, IndexUpdater } } - protected void verifyUpdates( Iterator> indexEntryUpdateIterator, int count ) + void verifyUpdates( Iterator> indexEntryUpdateIterator, int count ) throws IOException { @SuppressWarnings( "unchecked" ) @@ -821,7 +899,6 @@ public String toString() private RawCursor, IOException> scan( GBPTree tree ) throws IOException { -// tree.printTree( false, false, false ); KEY lowest = layout.newKey(); lowest.initAsLowest(); KEY highest = layout.newKey(); diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NonUniqueNativeSchemaIndexPopulatorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NonUniqueNativeSchemaIndexPopulatorTest.java index aacb1ca6fb02c..a107486b3961b 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NonUniqueNativeSchemaIndexPopulatorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NonUniqueNativeSchemaIndexPopulatorTest.java @@ -34,11 +34,9 @@ import org.neo4j.kernel.impl.api.index.sampling.IndexSamplingConfig; import org.neo4j.storageengine.api.schema.IndexSample; +import static java.util.Arrays.asList; import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; - -import static java.util.Arrays.asList; - import static org.neo4j.helpers.ArrayUtil.array; import static org.neo4j.index.internal.gbptree.RecoveryCleanupWorkCollector.IMMEDIATE; import static org.neo4j.kernel.impl.index.schema.FullScanNonUniqueIndexSamplerTest.countUniqueValues; @@ -109,27 +107,9 @@ public void shouldApplyLargeAmountOfInterleavedRandomUpdatesWithDuplicates() thr random.reset(); Random updaterRandom = new Random( random.seed() ); Iterator> updates = randomUniqueUpdateGenerator( random, 0.1f ); - int numberOfPopulatorUpdates = LARGE_AMOUNT_OF_UPDATES; // when - int count = 0; - for ( int i = 0; i < numberOfPopulatorUpdates; i++ ) - { - if ( updaterRandom.nextFloat() < 0.1 ) - { - try ( IndexUpdater indexUpdater = populator.newPopulatingUpdater( null_property_accessor ) ) - { - int numberOfUpdaterUpdates = updaterRandom.nextInt( 100 ); - for ( int j = 0; j < numberOfUpdaterUpdates; j++ ) - { - indexUpdater.process( updates.next() ); - count++; - } - } - } - populator.add( updates.next() ); - count++; - } + int count = interleaveLargeAmountOfUpdates( updaterRandom, updates ); // then populator.close( true ); diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/UniqueNativeSchemaIndexPopulatorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/UniqueNativeSchemaIndexPopulatorTest.java index 0d8540e35fe8f..73e71f0409107 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/UniqueNativeSchemaIndexPopulatorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/UniqueNativeSchemaIndexPopulatorTest.java @@ -122,14 +122,13 @@ public void shouldThrowOnLargeAmountOfInterleavedRandomUpdatesWithDuplicates() t random.reset(); Random updaterRandom = new Random( random.seed() ); Iterator> updates = randomUniqueUpdateGenerator( random, 0.01f ); - int numberOfPopulatorUpdates = LARGE_AMOUNT_OF_UPDATES; Number failSafeDuplicateValue = 12345.6789D; // when try { populator.add( add( 1_000_000_000, failSafeDuplicateValue ) ); - for ( int i = 0; i < numberOfPopulatorUpdates; i++ ) + for ( int i = 0; i < LARGE_AMOUNT_OF_UPDATES; i++ ) { if ( updaterRandom.nextFloat() < 0.1 ) {