From a4d82d341e5da145bc4b61f2a64601723e3b2db4 Mon Sep 17 00:00:00 2001 From: MishaDemianenko Date: Thu, 25 May 2017 18:44:07 +0200 Subject: [PATCH] Revert "Merge pull request #6 from davidegrohmann/3.3-page-cursors-in-property-cursors" This reverts commit 4b462d0273ff5eb3bc676fd1c81f414a7b5ed149, reversing changes made to 48c374cc6c304cfd7222843c95ae9caac0c25d58. --- .../kernel/impl/api/store/NodeCursor.java | 5 +- .../store/StoreAbstractPropertyCursor.java | 49 +++-------- .../impl/api/store/StorePropertyCursor.java | 5 +- .../api/store/StorePropertyPayloadCursor.java | 69 +++++----------- .../api/store/StoreSinglePropertyCursor.java | 5 +- .../kernel/impl/api/store/StoreStatement.java | 6 +- .../kernel/impl/store/RecordCursors.java | 18 ++++- .../participant/StoreMigrator.java | 24 +++--- .../kernel/impl/api/store/NodeCursorTest.java | 9 ++- .../StorageLayerRelTypesAndDegreeTest.java | 3 +- .../api/store/StorePropertyCursorTest.java | 81 +++++++++++-------- .../store/StorePropertyPayloadCursorTest.java | 62 +++++--------- .../kernel/impl/store/RecordCursorsTest.java | 2 + 13 files changed, 149 insertions(+), 189 deletions(-) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/NodeCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/NodeCursor.java index ce9497e2213f7..d927a21e66d52 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/NodeCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/NodeCursor.java @@ -61,10 +61,11 @@ public class NodeCursor implements NodeItem, Cursor, Disposable private Iterator added; private PageCursor pageCursor; - NodeCursor( NodeStore nodeStore, Consumer instanceCache, LockService lockService ) + NodeCursor( NodeRecord nodeRecord, Consumer instanceCache, NodeStore nodeStore, + LockService lockService ) { this.pageCursor = nodeStore.newPageCursor(); - this.nodeRecord = nodeStore.newRecord(); + this.nodeRecord = nodeRecord; this.instanceCache = instanceCache; this.nodeStore = nodeStore; this.lockService = lockService; diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreAbstractPropertyCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreAbstractPropertyCursor.java index 628847139a1e5..f4ff1a7047dcb 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreAbstractPropertyCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreAbstractPropertyCursor.java @@ -19,54 +19,46 @@ */ package org.neo4j.kernel.impl.api.store; -import java.io.IOException; import java.util.function.IntPredicate; import org.neo4j.cursor.Cursor; import org.neo4j.function.Disposable; -import org.neo4j.io.pagecache.PageCursor; import org.neo4j.kernel.api.properties.DefinedProperty; import org.neo4j.kernel.impl.locking.Lock; -import org.neo4j.kernel.impl.store.PropertyStore; -import org.neo4j.kernel.impl.store.UnderlyingStorageException; +import org.neo4j.kernel.impl.store.RecordCursor; +import org.neo4j.kernel.impl.store.RecordCursors; import org.neo4j.kernel.impl.store.record.PropertyRecord; import org.neo4j.kernel.impl.store.record.Record; import org.neo4j.storageengine.api.PropertyItem; import org.neo4j.storageengine.api.txstate.PropertyContainerState; -import static org.neo4j.kernel.api.StatementConstants.NO_SUCH_PROPERTY; import static org.neo4j.kernel.impl.store.record.RecordLoad.FORCE; public abstract class StoreAbstractPropertyCursor implements PropertyItem, Cursor, Disposable { protected final StorePropertyPayloadCursor payload; - private final PageCursor cursor; - private final PropertyRecord record; - private final PropertyStore propertyStore; + private final RecordCursor recordCursor; protected boolean fetched; private boolean doneTraversingTheChain; private DefinedProperty property; private IntPredicate propertyKeyIds; - private long nextPropertyId; private Lock lock; protected PropertyContainerState state; - StoreAbstractPropertyCursor( PropertyStore propertyStore ) + StoreAbstractPropertyCursor( RecordCursors cursors ) { - this.cursor = propertyStore.newPageCursor(); - this.propertyStore = propertyStore; - this.record = propertyStore.newRecord(); - this.payload = new StorePropertyPayloadCursor( propertyStore.getStringStore(), propertyStore.getArrayStore() ); + this.payload = new StorePropertyPayloadCursor( cursors.propertyString(), cursors.propertyArray() ); + this.recordCursor = cursors.property(); } protected final void initialize( IntPredicate propertyKeyIds, long firstPropertyId, Lock lock, PropertyContainerState state ) { this.propertyKeyIds = propertyKeyIds; - this.nextPropertyId = firstPropertyId; this.lock = lock; this.state = state; + recordCursor.placeAt( firstPropertyId, FORCE ); } @Override @@ -87,17 +79,16 @@ private boolean fetchNext() } // No, OK continue down the chain and hunt for more... - PropertyRecord propertyRecord = readRecord(); - nextPropertyId = propertyRecord.getNextProp(); - if ( propertyRecord.inUse() ) + if ( recordCursor.next() ) { + PropertyRecord propertyRecord = recordCursor.get(); payload.init( propertyKeyIds, propertyRecord.getBlocks(), propertyRecord.getNumberOfBlocks() ); if ( payloadHasNext() ) { return true; } } - else if ( Record.NO_NEXT_PROPERTY.is( nextPropertyId ) ) + else if ( Record.NO_NEXT_PROPERTY.is( recordCursor.get().getNextProp() ) ) { // No more records in this chain, i.e. no more properties. doneTraversingTheChain = true; @@ -111,23 +102,6 @@ else if ( Record.NO_NEXT_PROPERTY.is( nextPropertyId ) ) return (property = nextAdded()) != null; } - private PropertyRecord readRecord() - { - try - { - record.clear(); - if ( !Record.NO_NEXT_PROPERTY.is( nextPropertyId ) ) - { - propertyStore.readIntoRecord( nextPropertyId, record, FORCE, cursor ); - } - return record; - } - catch ( IOException e ) - { - throw new UnderlyingStorageException( e ); - } - } - private boolean payloadHasNext() { boolean next = payload.next(); @@ -182,7 +156,6 @@ public final void close() payload.close(); propertyKeyIds = null; property = null; - nextPropertyId = NO_SUCH_PROPERTY; doClose(); } finally @@ -197,7 +170,5 @@ public final void close() @Override public void dispose() { - cursor.close(); - payload.dispose(); } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyCursor.java index 912ca52d4210f..6a83698d14791 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyCursor.java @@ -24,7 +24,6 @@ import org.neo4j.kernel.api.properties.DefinedProperty; import org.neo4j.kernel.impl.locking.Lock; -import org.neo4j.kernel.impl.store.PropertyStore; import org.neo4j.kernel.impl.store.RecordCursors; import org.neo4j.storageengine.api.StorageProperty; import org.neo4j.storageengine.api.txstate.PropertyContainerState; @@ -40,9 +39,9 @@ public class StorePropertyCursor extends StoreAbstractPropertyCursor private Iterator storagePropertyIterator; - public StorePropertyCursor( PropertyStore propertyStore, Consumer instanceCache ) + public StorePropertyCursor( RecordCursors cursors, Consumer instanceCache ) { - super( propertyStore ); + super( cursors ); this.instanceCache = instanceCache; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java index 20aa78a8e60f1..a650af2dcec9b 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java @@ -19,20 +19,14 @@ */ package org.neo4j.kernel.impl.api.store; -import java.io.IOException; import java.nio.ByteBuffer; import java.nio.ByteOrder; import java.util.function.IntPredicate; -import org.neo4j.function.Disposable; -import org.neo4j.io.pagecache.PageCursor; -import org.neo4j.kernel.impl.store.CommonAbstractStore; -import org.neo4j.kernel.impl.store.DynamicArrayStore; -import org.neo4j.kernel.impl.store.DynamicStringStore; import org.neo4j.kernel.impl.store.LongerShortString; import org.neo4j.kernel.impl.store.PropertyType; +import org.neo4j.kernel.impl.store.RecordCursor; import org.neo4j.kernel.impl.store.ShortArray; -import org.neo4j.kernel.impl.store.UnderlyingStorageException; import org.neo4j.kernel.impl.store.record.DynamicRecord; import org.neo4j.kernel.impl.store.record.PropertyBlock; import org.neo4j.kernel.impl.store.record.Record; @@ -50,7 +44,7 @@ * During initialization the raw property block {@code long}s are read from * the given property record. */ -class StorePropertyPayloadCursor implements Disposable +class StorePropertyPayloadCursor { private static final int MAX_BYTES_IN_SHORT_STRING_OR_SHORT_ARRAY = 32; private static final int INTERNAL_BYTE_ARRAY_SIZE = 4096; @@ -61,11 +55,8 @@ class StorePropertyPayloadCursor implements Disposable */ private final ByteBuffer cachedBuffer = ByteBuffer.allocate( INTERNAL_BYTE_ARRAY_SIZE ); - private final DynamicRecord record; - private final PageCursor stringCursor; - private final DynamicStringStore stringStore; - private final PageCursor arrayCursor; - private final DynamicArrayStore arrayStore; + private final RecordCursor stringRecordCursor; + private final RecordCursor arrayRecordCursor; private ByteBuffer buffer = cachedBuffer; private long[] blocks; @@ -74,13 +65,11 @@ class StorePropertyPayloadCursor implements Disposable private IntPredicate propertyKeyIds; private boolean exhausted; - StorePropertyPayloadCursor( DynamicStringStore stringStore, DynamicArrayStore arrayStore ) + StorePropertyPayloadCursor( RecordCursor stringRecordCursor, + RecordCursor arrayRecordCursor ) { - this.record = stringStore.newRecord(); - this.stringStore = stringStore; - this.stringCursor = stringStore.newPageCursor(); - this.arrayStore = arrayStore; - this.arrayCursor = arrayStore.newPageCursor(); + this.stringRecordCursor = stringRecordCursor; + this.arrayRecordCursor = arrayRecordCursor; } void init( IntPredicate propertyKeyIds, long[] blocks, int numberOfBlocks ) @@ -174,7 +163,7 @@ Object value() return LongerShortString.decode( blocks, position, currentBlocksUsed() ); case STRING: { - readFromStore( stringStore, stringCursor ); + readFromStore( stringRecordCursor ); buffer.flip(); return UTF8.decode( buffer.array(), 0, buffer.limit() ); } @@ -182,7 +171,7 @@ Object value() return ShortArray.decode( valueAsBits() ); case ARRAY: { - readFromStore( arrayStore, arrayCursor ); + readFromStore( arrayRecordCursor ); buffer.flip(); return readArrayFromBuffer( buffer ); } @@ -212,14 +201,16 @@ private Bits valueAsBits() return bits; } - private void readFromStore( CommonAbstractStore store, PageCursor cursor ) + private void readFromStore( RecordCursor cursor ) { buffer.clear(); - long blockId = PropertyBlock.fetchLong( currentHeader() ); - do + long startBlockId = PropertyBlock.fetchLong( currentHeader() ); + cursor.placeAt( startBlockId, FORCE ); + while ( true ) { - readRecord( store, cursor, blockId ); - byte[] data = record.getData(); + cursor.next(); + DynamicRecord dynamicRecord = cursor.get(); + byte[] data = dynamicRecord.getData(); if ( buffer.remaining() < data.length ) { buffer.flip(); @@ -228,21 +219,10 @@ private void readFromStore( CommonAbstractStore store, PageCurs buffer = newBuffer; } buffer.put( data, 0, data.length ); - blockId = record.getNextBlock(); - } - while ( !Record.NULL_REFERENCE.is( blockId ) ); - } - - private void readRecord( CommonAbstractStore store, PageCursor cursor, long blockId ) - { - try - { - record.clear(); - store.readIntoRecord( blockId, record, FORCE, cursor ); - } - catch ( IOException e ) - { - throw new UnderlyingStorageException( e ); + if ( Record.NULL_REFERENCE.is( dynamicRecord.getNextBlock() ) ) + { + break; + } } } @@ -312,11 +292,4 @@ private static Object readArrayFromBuffer( ByteBuffer buffer ) buffer.order( ByteOrder.LITTLE_ENDIAN ); } } - - @Override - public void dispose() - { - stringCursor.close(); - arrayCursor.close(); - } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreSinglePropertyCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreSinglePropertyCursor.java index b5158c5b58f7b..9c6166cfeb657 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreSinglePropertyCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreSinglePropertyCursor.java @@ -23,7 +23,6 @@ import org.neo4j.kernel.api.properties.DefinedProperty; import org.neo4j.kernel.impl.locking.Lock; -import org.neo4j.kernel.impl.store.PropertyStore; import org.neo4j.kernel.impl.store.RecordCursors; import org.neo4j.storageengine.api.txstate.PropertyContainerState; @@ -32,9 +31,9 @@ public class StoreSinglePropertyCursor extends StoreAbstractPropertyCursor private final Consumer instanceCache; private int propertyKeyId; - StoreSinglePropertyCursor( PropertyStore propertyStore, Consumer instanceCache ) + StoreSinglePropertyCursor( RecordCursors cursors, Consumer instanceCache ) { - super( propertyStore ); + super( cursors ); this.instanceCache = instanceCache; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreStatement.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreStatement.java index e3bd9b8b39cc3..e772412ee801c 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreStatement.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreStatement.java @@ -89,7 +89,7 @@ public StoreStatement( NeoStores neoStores, Supplier indexRe @Override protected NodeCursor create() { - return new NodeCursor( nodeStore, this, lockService ); + return new NodeCursor( nodeStore.newRecord(), this, nodeStore, lockService ); } }; singleRelationshipCursor = new InstanceCache() @@ -124,7 +124,7 @@ protected StoreNodeRelationshipCursor create() @Override protected StorePropertyCursor create() { - return new StorePropertyCursor( neoStores.getPropertyStore(), this ); + return new StorePropertyCursor( recordCursors, this ); } }; singlePropertyCursorCache = new InstanceCache() @@ -132,7 +132,7 @@ protected StorePropertyCursor create() @Override protected StoreSinglePropertyCursor create() { - return new StoreSinglePropertyCursor( neoStores.getPropertyStore(), this ); + return new StoreSinglePropertyCursor( recordCursors, this ); } }; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordCursors.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordCursors.java index bbdb709a4bc23..a1c0b098c3beb 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordCursors.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordCursors.java @@ -21,6 +21,7 @@ import org.neo4j.io.IOUtils; import org.neo4j.kernel.impl.store.record.AbstractBaseRecord; +import org.neo4j.kernel.impl.store.record.DynamicRecord; import org.neo4j.kernel.impl.store.record.PropertyRecord; import org.neo4j.kernel.impl.store.record.RelationshipGroupRecord; import org.neo4j.kernel.impl.store.record.RelationshipRecord; @@ -35,12 +36,16 @@ public class RecordCursors implements AutoCloseable private final RecordCursor relationship; private final RecordCursor relationshipGroup; private final RecordCursor property; + private final RecordCursor propertyString; + private final RecordCursor propertyArray; public RecordCursors( NeoStores neoStores ) { relationship = newCursor( neoStores.getRelationshipStore() ); relationshipGroup = newCursor( neoStores.getRelationshipGroupStore() ); property = newCursor( neoStores.getPropertyStore() ); + propertyString = newCursor( neoStores.getPropertyStore().getStringStore() ); + propertyArray = newCursor( neoStores.getPropertyStore().getArrayStore() ); } private static RecordCursor newCursor( RecordStore store ) @@ -51,7 +56,8 @@ private static RecordCursor newCursor( RecordS @Override public void close() { - IOUtils.closeAll( RuntimeException.class, relationship, relationshipGroup, property ); + IOUtils.closeAll( RuntimeException.class, relationship, relationshipGroup, property, propertyArray, + propertyString ); } public RecordCursor relationship() @@ -68,4 +74,14 @@ public RecordCursor property() { return property; } + + public RecordCursor propertyArray() + { + return propertyArray; + } + + public RecordCursor propertyString() + { + return propertyString; + } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/participant/StoreMigrator.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/participant/StoreMigrator.java index 0df9a861c91e7..cf20e81eaac7e 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/participant/StoreMigrator.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/participant/StoreMigrator.java @@ -57,7 +57,7 @@ import org.neo4j.kernel.impl.store.MetaDataStore.Position; import org.neo4j.kernel.impl.store.NeoStores; import org.neo4j.kernel.impl.store.NodeStore; -import org.neo4j.kernel.impl.store.PropertyStore; +import org.neo4j.kernel.impl.store.RecordCursors; import org.neo4j.kernel.impl.store.RelationshipStore; import org.neo4j.kernel.impl.store.StoreFactory; import org.neo4j.kernel.impl.store.StoreFile; @@ -365,6 +365,8 @@ private void migrateWithBatchImporter( File storeDir, File migrationDir, long la !newFormat.property().equals( oldFormat.property() ) || requiresDynamicStoreMigration; File badFile = new File( storeDir, Configuration.BAD_FILE_NAME ); try ( NeoStores legacyStore = instantiateLegacyStore( oldFormat, storeDir ); + RecordCursors nodeInputCursors = new RecordCursors( legacyStore ); + RecordCursors relationshipInputCursors = new RecordCursors( legacyStore ); OutputStream badOutput = new BufferedOutputStream( new FileOutputStream( badFile, false ) ) ) { Configuration importConfig = new Configuration.Overridden( config ); @@ -376,9 +378,10 @@ private void migrateWithBatchImporter( File storeDir, File migrationDir, long la importConfig, logService, withDynamicProcessorAssignment( migrationBatchImporterMonitor( legacyStore, progressMonitor, importConfig ), importConfig ), additionalInitialIds, config, newFormat ); - InputIterable nodes = legacyNodesAsInput( legacyStore, requiresPropertyMigration ); + InputIterable nodes = + legacyNodesAsInput( legacyStore, requiresPropertyMigration, nodeInputCursors ); InputIterable relationships = - legacyRelationshipsAsInput( legacyStore, requiresPropertyMigration ); + legacyRelationshipsAsInput( legacyStore, requiresPropertyMigration, relationshipInputCursors ); importer.doImport( Inputs.input( nodes, relationships, IdMappers.actual(), IdGenerators.fromInput(), Collectors.badCollector( badOutput, 0 ) ) ); @@ -563,11 +566,11 @@ private ExecutionMonitor migrationBatchImporterMonitor( NeoStores legacyStore, } private InputIterable legacyRelationshipsAsInput( NeoStores legacyStore, - boolean requiresPropertyMigration ) + boolean requiresPropertyMigration, RecordCursors cursors ) { RelationshipStore store = legacyStore.getRelationshipStore(); final BiConsumer propertyDecorator = - propertyDecorator( requiresPropertyMigration, legacyStore.getPropertyStore() ); + propertyDecorator( requiresPropertyMigration, cursors ); return new StoreScanAsInputIterable( store ) { @Override @@ -583,11 +586,12 @@ protected InputRelationship inputEntityOf( RelationshipRecord record ) }; } - private InputIterable legacyNodesAsInput( NeoStores legacyStore, boolean requiresPropertyMigration ) + private InputIterable legacyNodesAsInput( NeoStores legacyStore, + boolean requiresPropertyMigration, RecordCursors cursors ) { NodeStore store = legacyStore.getNodeStore(); - BiConsumer propertyDecorator = - propertyDecorator( requiresPropertyMigration, legacyStore.getPropertyStore() ); + final BiConsumer propertyDecorator = + propertyDecorator( requiresPropertyMigration, cursors ); return new StoreScanAsInputIterable( store ) { @@ -605,7 +609,7 @@ protected InputNode inputEntityOf( NodeRecord record ) } private BiConsumer propertyDecorator( - boolean requiresPropertyMigration, PropertyStore propertyStore ) + boolean requiresPropertyMigration, RecordCursors cursors ) { if ( !requiresPropertyMigration ) { @@ -614,7 +618,7 @@ private BiConsumer< }; } - final StorePropertyCursor cursor = new StorePropertyCursor( propertyStore, ignored -> {} ); + final StorePropertyCursor cursor = new StorePropertyCursor( cursors, ignored -> {} ); final List scratch = new ArrayList<>(); return ( ENTITY entity, RECORD record ) -> { diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/NodeCursorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/NodeCursorTest.java index 3386f94fbe505..f66dd11615ef9 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/NodeCursorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/NodeCursorTest.java @@ -79,11 +79,10 @@ public class NodeCursorTest private final PageCursor pageCursor = mock( PageCursor.class ); { when( nodeStore.newPageCursor() ).thenReturn( pageCursor ); - when( nodeStore.newRecord() ).thenReturn( nodeRecord ); } // cursor is shared since it is designed to be reusable - private final NodeCursor reusableCursor = new NodeCursor( nodeStore, i -> {}, NO_LOCK_SERVICE ); + private final NodeCursor reusableCursor = new NodeCursor( nodeRecord, i -> {}, nodeStore, NO_LOCK_SERVICE ); @SuppressWarnings( "unchecked" ) @DataPoints @@ -222,7 +221,8 @@ public void fourElementsInFetchMode( LongFunction op0, LongFunction called.setTrue(), NO_LOCK_SERVICE ); + NodeCursor cursor = + new NodeCursor( nodeRecord, c -> called.setTrue(), nodeStore, NO_LOCK_SERVICE ); cursor.init( mock( NodeProgression.class ), mock( ReadableTransactionState.class ) ); assertFalse( called.booleanValue() ); @@ -233,7 +233,8 @@ public void shouldCallTheConsumerOnClose() @Test public void shouldCloseThePageCursorWhenDisposed() { - NodeCursor cursor = new NodeCursor( nodeStore, c -> {}, NO_LOCK_SERVICE ); + NodeCursor cursor = + new NodeCursor( nodeRecord, c -> {}, nodeStore, NO_LOCK_SERVICE ); cursor.init( mock( NodeProgression.class ), mock( ReadableTransactionState.class ) ); cursor.close(); diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorageLayerRelTypesAndDegreeTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorageLayerRelTypesAndDegreeTest.java index 5eea2e630493e..98999d905aa4d 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorageLayerRelTypesAndDegreeTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorageLayerRelTypesAndDegreeTest.java @@ -473,7 +473,8 @@ private Set degrees( NodeItem nodeItem ) private NodeCursor newCursor( long nodeId ) { NodeCursor cursor = - new NodeCursor( resolveNeoStores().getNodeStore(), mock( Consumer.class ), NO_LOCK_SERVICE ); + new NodeCursor( new NodeRecord( -1 ), mock( Consumer.class ), resolveNeoStores().getNodeStore(), + NO_LOCK_SERVICE ); cursor.init( new SingleNodeProgression( nodeId ), null ); assertTrue( cursor.next() ); return cursor; diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyCursorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyCursorTest.java index ca430658bdad1..ee84251b1f546 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyCursorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyCursorTest.java @@ -38,13 +38,14 @@ import org.neo4j.cursor.Cursor; import org.neo4j.graphdb.mockfs.EphemeralFileSystemAbstraction; import org.neo4j.io.pagecache.PageCache; -import org.neo4j.io.pagecache.PageCursor; import org.neo4j.kernel.impl.store.DynamicArrayStore; import org.neo4j.kernel.impl.store.DynamicRecordAllocator; import org.neo4j.kernel.impl.store.DynamicStringStore; import org.neo4j.kernel.impl.store.NeoStores; import org.neo4j.kernel.impl.store.PropertyStore; import org.neo4j.kernel.impl.store.PropertyType; +import org.neo4j.kernel.impl.store.RecordCursor; +import org.neo4j.kernel.impl.store.RecordCursors; import org.neo4j.kernel.impl.store.RecordStore; import org.neo4j.kernel.impl.store.StoreFactory; import org.neo4j.kernel.impl.store.format.standard.PropertyRecordFormat; @@ -54,6 +55,7 @@ import org.neo4j.kernel.impl.store.record.PropertyRecord; import org.neo4j.logging.NullLogProvider; import org.neo4j.storageengine.api.PropertyItem; +import org.neo4j.test.MockedNeoStores; import org.neo4j.test.rule.PageCacheRule; import org.neo4j.test.rule.fs.EphemeralFileSystemRule; @@ -65,11 +67,13 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.neo4j.kernel.impl.locking.LockService.NO_LOCK; +import static org.neo4j.kernel.impl.store.record.RecordLoad.NORMAL; @RunWith( Enclosed.class ) public class StorePropertyCursorTest @@ -224,26 +228,32 @@ private static Object actualValue( Object parameter ) return parameter instanceof BigProperty ? ((BigProperty)parameter).value() : parameter; } - public static class CloseDisposeTest + public static class ErrorTest { - private final PropertyStore propertyStore = mock( PropertyStore.class ); - private final DynamicStringStore stringStore = mock( DynamicStringStore.class ); - private final DynamicArrayStore arrayStore = mock( DynamicArrayStore.class ); + private final NeoStores neoStores = MockedNeoStores.basicMockedNeoStores(); + private final PropertyStore propertyStore = neoStores.getPropertyStore(); + @SuppressWarnings( "unchecked" ) + private final Consumer cache = mock( Consumer.class ); + { - when( stringStore.newRecord() ).thenReturn( mock( DynamicRecord.class ) ); - when( propertyStore.getStringStore() ).thenReturn( stringStore ); - when( arrayStore.newRecord() ).thenReturn( mock( DynamicRecord.class ) ); - when( propertyStore.getArrayStore() ).thenReturn( arrayStore ); + RecordCursor recordCursor = MockedNeoStores.mockedRecordCursor(); + try + { + when( recordCursor.next() ).thenReturn( true ); + } + catch ( Exception e ) + { + throw new RuntimeException( e ); + } + when( recordCursor.get() ).thenReturn( new PropertyRecord( 42 ) ); + when( propertyStore.newRecordCursor( any( PropertyRecord.class ) ) ).thenReturn( recordCursor ); } @Test public void shouldReturnTheCursorToTheCacheOnClose() throws Throwable { // given - @SuppressWarnings( "unchecked" ) - Consumer cache = mock( Consumer.class ); - - StorePropertyCursor storePropertyCursor = new StorePropertyCursor( propertyStore, cache ); + StorePropertyCursor storePropertyCursor = newStorePropertyCursor( propertyStore, cache ); storePropertyCursor.init( 0, NO_LOCK, null ); // when @@ -252,27 +262,6 @@ public void shouldReturnTheCursorToTheCacheOnClose() throws Throwable // then verify( cache, times( 1 ) ).accept( storePropertyCursor ); } - - @Test - public void shouldClosePageCursorsOnDispose() throws Throwable - { - // given - PageCursor propertyCursor = mock( PageCursor.class ); - when( propertyStore.newPageCursor() ).thenReturn( propertyCursor ); - PageCursor stringCursor = mock( PageCursor.class ); - when( stringStore.newPageCursor() ).thenReturn( stringCursor ); - PageCursor arrayCursor = mock( PageCursor.class ); - when( arrayStore.newPageCursor() ).thenReturn( arrayCursor ); - StorePropertyCursor storePropertyCursor = new StorePropertyCursor( propertyStore, c -> {} ); - - // when - storePropertyCursor.dispose(); - - // then - verify( propertyCursor ).close(); - verify( stringCursor ).close(); - verify( arrayCursor ).close(); - } } public static class PropertyStoreBasedTestSupport @@ -764,7 +753,29 @@ public static long firstIdOf( List propertyChain ) private static StorePropertyCursor newStorePropertyCursor( PropertyStore propertyStore ) { - return new StorePropertyCursor( propertyStore, ignored -> {} ); + return newStorePropertyCursor( propertyStore, ignored -> {} ); + } + + private static StorePropertyCursor newStorePropertyCursor( PropertyStore propertyStore, + Consumer cache ) + { + RecordCursor propertyRecordCursor = propertyStore.newRecordCursor( propertyStore.newRecord() ); + propertyRecordCursor.acquire( 0, NORMAL ); + + DynamicStringStore stringStore = propertyStore.getStringStore(); + RecordCursor dynamicStringCursor = stringStore.newRecordCursor( stringStore.nextRecord() ); + dynamicStringCursor.acquire( 0, NORMAL ); + + DynamicArrayStore arrayStore = propertyStore.getArrayStore(); + RecordCursor dynamicArrayCursor = arrayStore.newRecordCursor( arrayStore.nextRecord() ); + dynamicArrayCursor.acquire( 0, NORMAL ); + + RecordCursors cursors = mock( RecordCursors.class ); + when( cursors.property() ).thenReturn( propertyRecordCursor ); + when( cursors.propertyString() ).thenReturn( dynamicStringCursor ); + when( cursors.propertyArray() ).thenReturn( dynamicArrayCursor ); + + return new StorePropertyCursor( cursors, cache ); } private static List createPropertyChain( PropertyStore store, int keyId, diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursorTest.java index 0306dd62ed714..3bab3ea24d6d2 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursorTest.java @@ -26,7 +26,6 @@ import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameter; -import java.io.IOException; import java.util.Arrays; import java.util.Iterator; import java.util.List; @@ -35,13 +34,13 @@ import org.neo4j.helpers.Strings; import org.neo4j.helpers.collection.Iterables; import org.neo4j.helpers.collection.Iterators; -import org.neo4j.io.pagecache.PageCursor; import org.neo4j.kernel.impl.store.AbstractDynamicStore; import org.neo4j.kernel.impl.store.DynamicArrayStore; import org.neo4j.kernel.impl.store.DynamicRecordAllocator; import org.neo4j.kernel.impl.store.DynamicStringStore; import org.neo4j.kernel.impl.store.PropertyStore; import org.neo4j.kernel.impl.store.PropertyType; +import org.neo4j.kernel.impl.store.RecordCursor; import org.neo4j.kernel.impl.store.StandaloneDynamicRecordAllocator; import org.neo4j.kernel.impl.store.record.DynamicRecord; import org.neo4j.kernel.impl.store.record.PropertyBlock; @@ -53,7 +52,6 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyLong; -import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -84,7 +82,7 @@ public void nextShouldAlwaysReturnFalseWhenNotInitialized() { @SuppressWarnings( "unchecked" ) StorePropertyPayloadCursor cursor = - new StorePropertyPayloadCursor( mock( DynamicStringStore.class ), mock( DynamicArrayStore.class ) ); + new StorePropertyPayloadCursor( mock( RecordCursor.class ), mock( RecordCursor.class ) ); assertFalse( cursor.next() ); @@ -183,7 +181,7 @@ public void shouldBePossibleToCallNextOnEmptyCursor() } @Test - public void shouldUseDynamicStringAndArrayStoresThroughDifferentCursors() throws Throwable + public void shouldUseDynamicStringAndArrayStoresThroughDifferentCursors() { // Given DynamicStringStore dynamicStringStore = newDynamicStoreMock( DynamicStringStore.class ); @@ -204,8 +202,8 @@ public void shouldUseDynamicStringAndArrayStoresThroughDifferentCursors() throws assertFalse( cursor.next() ); // Then - verify( dynamicStringStore ).newPageCursor(); - verify( dynamicArrayStore ).newPageCursor(); + verify( dynamicStringStore ).newRecordCursor( any( DynamicRecord.class ) ); + verify( dynamicArrayStore ).newRecordCursor( any( DynamicRecord.class ) ); } @Test @@ -219,26 +217,6 @@ public void nextMultipleInvocations() assertFalse( cursor.next() ); } - @Test - public void shouldClosePageCursorWhenDisposed() - { - // given - DynamicStringStore stringStore = mock( DynamicStringStore.class ); - PageCursor stringCursor = mock( PageCursor.class ); - when( stringStore.newPageCursor() ).thenReturn( stringCursor ); - DynamicArrayStore arrayStore = mock( DynamicArrayStore.class ); - PageCursor arrayCursor = mock( PageCursor.class ); - when( arrayStore.newPageCursor() ).thenReturn( arrayCursor ); - StorePropertyPayloadCursor cursor = createCursor( ALWAYS_TRUE_INT, stringStore, arrayStore, new Param[0] ); - cursor.close(); - - // when - cursor.dispose(); - - // then - verify( stringCursor ).close(); - verify( arrayCursor ).close(); - } } @RunWith( Parameterized.class ) @@ -498,12 +476,16 @@ private static StorePropertyPayloadCursor singleProperty( int keyId, Params inpu return createCursor( k -> k == keyId, dynamicStringStore, dynamicArrayStore, input.params ); } - private static StorePropertyPayloadCursor createCursor( IntPredicate allPropertyKeyIds, - DynamicStringStore dynamicStringStore, DynamicArrayStore dynamicArrayStore, Param[] params ) + private static StorePropertyPayloadCursor createCursor( IntPredicate allPropertyKeyIds, DynamicStringStore + dynamicStringStore, + DynamicArrayStore dynamicArrayStore, Param[] params ) { - StorePropertyPayloadCursor cursor = new StorePropertyPayloadCursor( dynamicStringStore, dynamicArrayStore ); + StorePropertyPayloadCursor cursor = new StorePropertyPayloadCursor( + dynamicStringStore.newRecordCursor( null ), dynamicArrayStore.newRecordCursor( null ) ); + long[] blocks = asBlocks( params ); cursor.init( allPropertyKeyIds, blocks, blocks.length ); + return cursor; } @@ -527,18 +509,18 @@ private static long[] asBlocks( Param... params ) } @SuppressWarnings( "unchecked" ) - private static S newDynamicStoreMock( Class clazz ) throws IOException + private static S newDynamicStoreMock( Class clazz ) { + RecordCursor recordCursor = mock( RecordCursor.class ); + when( recordCursor.next() ).thenReturn( true ).thenReturn( false ); + DynamicRecord dynamicRecord = new DynamicRecord( 42 ); + dynamicRecord.setData( new byte[]{1, 1, 1, 1, 1} ); + when( recordCursor.get() ).thenReturn( dynamicRecord ); + when( recordCursor.acquire( anyLong(), any( RecordLoad.class ) ) ).thenReturn( recordCursor ); + S store = mock( clazz ); - when( store.newRecord() ).thenReturn( new DynamicRecord( DynamicRecord.NO_ID ) ); - doAnswer( invocationOnMock -> - { - DynamicRecord record = (DynamicRecord) invocationOnMock.getArguments()[1]; - record.setId( 42 ); - record.setData( new byte[]{1, 1, 1, 1, 1} ); - return null; - } ).when( store ).readIntoRecord( anyLong(), any( DynamicRecord.class ), any( RecordLoad.class ), - any( PageCursor.class ) ); + when( store.newRecordCursor( any( DynamicRecord.class ) ) ).thenReturn( recordCursor ); + return store; } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/RecordCursorsTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/RecordCursorsTest.java index e749b83923438..8655970eb7827 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/RecordCursorsTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/RecordCursorsTest.java @@ -69,6 +69,8 @@ private static void verifyAllCursorsClosed( RecordCursors recordCursors ) verify( recordCursors.relationship() ).close(); verify( recordCursors.relationshipGroup() ).close(); verify( recordCursors.property() ).close(); + verify( recordCursors.propertyString() ).close(); + verify( recordCursors.propertyArray() ).close(); } private static RecordCursors newRecordCursorsWithMockedNeoStores()