Skip to content

Commit

Permalink
Fixes from code review
Browse files Browse the repository at this point in the history
- set read to null on close in cursors
- some style
- some documentation
  • Loading branch information
pontusmelke committed Dec 4, 2017
1 parent 7b0ccd5 commit 3d45cc2
Show file tree
Hide file tree
Showing 20 changed files with 48 additions and 28 deletions.
Expand Up @@ -62,6 +62,8 @@ enum Type


/** /**
* @return The Write operations of the graph. * @return The Write operations of the graph.
* @throws InvalidTransactionTypeKernelException when transaction cannot be upgraded to a write transaction. This
* can happen when there have been schema modifications.
*/ */
Write dataWrite() throws InvalidTransactionTypeKernelException; Write dataWrite() throws InvalidTransactionTypeKernelException;


Expand Down
Expand Up @@ -84,7 +84,7 @@ public Value propertyValue()
@Override @Override
public <E extends Exception> void writeTo( ValueWriter<E> target ) public <E extends Exception> void writeTo( ValueWriter<E> target )
{ {

throw new UnsupportedOperationException( "not implemented" );
} }


@Override @Override
Expand Down
Expand Up @@ -199,7 +199,7 @@ public KernelTransactionImplementation( StatementOperationParts statementOperati
this.userMetaData = new HashMap<>(); this.userMetaData = new HashMap<>();
AllStoreHolder allStoreHolder = AllStoreHolder allStoreHolder =
new AllStoreHolder( storageEngine, storageStatement, this, cursors, explicitIndexStore, new AllStoreHolder( storageEngine, storageStatement, this, cursors, explicitIndexStore,
this::assertTransactionOpen ); this::assertOpen );
this.operations = this.operations =
new Operations( new Operations(
allStoreHolder, allStoreHolder,
Expand Down Expand Up @@ -462,6 +462,15 @@ private void assertTransactionOpen()
} }
} }


private void assertOpen()
{
Optional<Status> terminationReason = getReasonIfTerminated();
if ( terminationReason.isPresent() )
{
throw new TransactionTerminatedException( terminationReason.get() );
}
}

private boolean hasChanges() private boolean hasChanges()
{ {
return hasTxStateWithChanges() || hasExplicitIndexChanges(); return hasTxStateWithChanges() || hasExplicitIndexChanges();
Expand Down
Expand Up @@ -449,9 +449,9 @@ public Map<String,Object> getAllProperties()
CursorFactory cursors = transaction.cursors(); CursorFactory cursors = transaction.cursors();
Map<String,Object> properties = new HashMap<>(); Map<String,Object> properties = new HashMap<>();


try ( NodeCursor nodes = cursors.allocateNodeCursor(); try ( Statement ignore = actions.statement();
PropertyCursor propertyCursor = cursors.allocatePropertyCursor(); NodeCursor nodes = cursors.allocateNodeCursor();
Statement ignore = actions.statement() ) PropertyCursor propertyCursor = cursors.allocatePropertyCursor() )
{ {
Token token = transaction.token(); Token token = transaction.token();
transaction.dataRead().singleNode( nodeId, nodes ); transaction.dataRead().singleNode( nodeId, nodes );
Expand Down
Expand Up @@ -332,8 +332,9 @@ public Node getNodeById( long id )
} }


KernelTransaction ktx = spi.currentTransaction(); KernelTransaction ktx = spi.currentTransaction();
try ( NodeCursor nodeCursor = ktx.cursors().allocateNodeCursor(); try ( Statement ignore = spi.currentStatement();
Statement ignore = spi.currentStatement() ) NodeCursor nodeCursor = ktx.cursors().allocateNodeCursor()
)
{ {
ktx.dataRead().singleNode( id, nodeCursor ); ktx.dataRead().singleNode( id, nodeCursor );
if ( !nodeCursor.next() ) if ( !nodeCursor.next() )
Expand Down
Expand Up @@ -302,7 +302,7 @@ ArrayValue array( PropertyCursor cursor, long reference, PageCursor page )
return PropertyUtil.readArrayFromBuffer( buffer ); return PropertyUtil.readArrayFromBuffer( buffer );
} }


public boolean nodeExists( long id ) public boolean nodeExistsInStore( long id )
{ {
return storeReadLayer.nodeExists( id ); return storeReadLayer.nodeExists( id );
} }
Expand Down
Expand Up @@ -19,13 +19,10 @@
*/ */
package org.neo4j.kernel.impl.newapi; package org.neo4j.kernel.impl.newapi;


import java.util.function.Supplier;

import org.neo4j.internal.kernel.api.CursorFactory; import org.neo4j.internal.kernel.api.CursorFactory;
import org.neo4j.internal.kernel.api.Kernel; import org.neo4j.internal.kernel.api.Kernel;
import org.neo4j.internal.kernel.api.security.SecurityContext; import org.neo4j.internal.kernel.api.security.SecurityContext;
import org.neo4j.kernel.api.InwardKernel; import org.neo4j.kernel.api.InwardKernel;
import org.neo4j.kernel.api.txstate.ExplicitIndexTransactionState;
import org.neo4j.storageengine.api.StorageEngine; import org.neo4j.storageengine.api.StorageEngine;
import org.neo4j.storageengine.api.StorageStatement; import org.neo4j.storageengine.api.StorageStatement;


Expand All @@ -48,7 +45,6 @@ public NewKernel( StorageEngine engine, InwardKernel kernel )
this.engine = engine; this.engine = engine;
this.kernel = kernel; this.kernel = kernel;
this.isRunning = false; this.isRunning = false;
this.cursors = new Cursors( );
} }


@Override @Override
Expand Down Expand Up @@ -80,5 +76,6 @@ public void stop()
} }
statement.close(); statement.close();
isRunning = false; isRunning = false;
this.cursors = null;
} }
} }
Expand Up @@ -271,6 +271,9 @@ public void close()
pageCursor.close(); pageCursor.close();
pageCursor = null; pageCursor = null;
} }
read = null;
labelCursor = null;

reset(); reset();
} }


Expand Down
Expand Up @@ -89,6 +89,7 @@ public void close()
node = NO_ID; node = NO_ID;
score = 0; score = 0;
expectedSize = 0; expectedSize = 0;
read = null;
} }


@Override @Override
Expand Down
Expand Up @@ -81,6 +81,7 @@ public void close()
super.close(); super.close();
node = NO_ID; node = NO_ID;
labels = null; labels = null;
read = null;
} }


@Override @Override
Expand Down
Expand Up @@ -102,6 +102,7 @@ public void close()
this.node = NO_ID; this.node = NO_ID;
this.keys = null; this.keys = null;
this.values = null; this.values = null;
this.read = null;
} }


@Override @Override
Expand Down
Expand Up @@ -367,7 +367,7 @@ public boolean nodeDelete( long node ) throws AutoIndexingKernelException
} }


ktx.locks().optimistic().acquireExclusive( ktx.lockTracer(), ResourceTypes.NODE, node ); ktx.locks().optimistic().acquireExclusive( ktx.lockTracer(), ResourceTypes.NODE, node );
if ( allStoreHolder.nodeExists( node ) ) if ( allStoreHolder.nodeExistsInStore( node ) )
{ {
autoIndexing.nodes().entityRemoved( this, node ); autoIndexing.nodes().entityRemoved( this, node );
ktx.txState().nodeDoDelete( node ); ktx.txState().nodeDoDelete( node );
Expand Down
Expand Up @@ -196,6 +196,7 @@ public void close()
propertiesState = null; propertiesState = null;
changedProperties = null; changedProperties = null;
stateValue = null; stateValue = null;
read = null;
clear(); clear();
} }


Expand Down
Expand Up @@ -25,7 +25,6 @@
import org.neo4j.internal.kernel.api.CapableIndexReference; import org.neo4j.internal.kernel.api.CapableIndexReference;
import org.neo4j.internal.kernel.api.IndexOrder; import org.neo4j.internal.kernel.api.IndexOrder;
import org.neo4j.internal.kernel.api.IndexQuery; import org.neo4j.internal.kernel.api.IndexQuery;
import org.neo4j.internal.kernel.api.RelationshipExplicitIndexCursor;
import org.neo4j.internal.kernel.api.Scan; import org.neo4j.internal.kernel.api.Scan;
import org.neo4j.internal.kernel.api.exceptions.KernelException; import org.neo4j.internal.kernel.api.exceptions.KernelException;
import org.neo4j.io.pagecache.PageCursor; import org.neo4j.io.pagecache.PageCursor;
Expand Down Expand Up @@ -165,7 +164,7 @@ public final Scan<org.neo4j.internal.kernel.api.NodeLabelIndexCursor> nodeLabelS
@Override @Override
public final void allNodesScan( org.neo4j.internal.kernel.api.NodeCursor cursor ) public final void allNodesScan( org.neo4j.internal.kernel.api.NodeCursor cursor )
{ {
((NodeCursor) cursor).scan(this); ((NodeCursor) cursor).scan( this );
} }


@Override @Override
Expand Down Expand Up @@ -319,52 +318,52 @@ public final void nodeExplicitIndexQuery(
org.neo4j.internal.kernel.api.NodeExplicitIndexCursor cursor, String index, String key, Object query ) throws KernelException org.neo4j.internal.kernel.api.NodeExplicitIndexCursor cursor, String index, String key, Object query ) throws KernelException
{ {
((NodeExplicitIndexCursor) cursor).setRead( this ); ((NodeExplicitIndexCursor) cursor).setRead( this );
explicitIndex( (org.neo4j.kernel.impl.newapi.NodeExplicitIndexCursor) cursor, explicitNodeIndex( index ).query( explicitIndex( (NodeExplicitIndexCursor) cursor, explicitNodeIndex( index ).query(
key, query instanceof Value ? ((Value) query).asObject() : query ) ); key, query instanceof Value ? ((Value) query).asObject() : query ) );
} }


@Override @Override
public void relationshipExplicitIndexGet( public void relationshipExplicitIndexGet(
RelationshipExplicitIndexCursor cursor, org.neo4j.internal.kernel.api.RelationshipExplicitIndexCursor cursor,
String index, String index,
String key, String key,
Value value, Value value,
long source, long source,
long target ) throws KernelException long target ) throws KernelException
{ {
((org.neo4j.kernel.impl.newapi.RelationshipExplicitIndexCursor) cursor).setRead( this ); ((RelationshipExplicitIndexCursor) cursor).setRead( this );
explicitIndex( explicitIndex(
(org.neo4j.kernel.impl.newapi.RelationshipExplicitIndexCursor) cursor, (RelationshipExplicitIndexCursor) cursor,
explicitRelationshipIndex( index ).get( key, value.asObject(), source, target ) ); explicitRelationshipIndex( index ).get( key, value.asObject(), source, target ) );
} }


@Override @Override
public void relationshipExplicitIndexQuery( public void relationshipExplicitIndexQuery(
RelationshipExplicitIndexCursor cursor, org.neo4j.internal.kernel.api.RelationshipExplicitIndexCursor cursor,
String index, String index,
Object query, Object query,
long source, long source,
long target ) throws KernelException long target ) throws KernelException
{ {
((org.neo4j.kernel.impl.newapi.RelationshipExplicitIndexCursor) cursor).setRead( this ); ((RelationshipExplicitIndexCursor) cursor).setRead( this );
explicitIndex( explicitIndex(
(org.neo4j.kernel.impl.newapi.RelationshipExplicitIndexCursor) cursor, (RelationshipExplicitIndexCursor) cursor,
explicitRelationshipIndex( index ) explicitRelationshipIndex( index )
.query( query instanceof Value ? ((Value) query).asObject() : query, source, target ) ); .query( query instanceof Value ? ((Value) query).asObject() : query, source, target ) );
} }


@Override @Override
public void relationshipExplicitIndexQuery( public void relationshipExplicitIndexQuery(
RelationshipExplicitIndexCursor cursor, org.neo4j.internal.kernel.api.RelationshipExplicitIndexCursor cursor,
String index, String index,
String key, String key,
Object query, Object query,
long source, long source,
long target ) throws KernelException long target ) throws KernelException
{ {
((org.neo4j.kernel.impl.newapi.RelationshipExplicitIndexCursor) cursor).setRead( this ); ((RelationshipExplicitIndexCursor) cursor).setRead( this );
explicitIndex( explicitIndex(
(org.neo4j.kernel.impl.newapi.RelationshipExplicitIndexCursor) cursor, (RelationshipExplicitIndexCursor) cursor,
explicitRelationshipIndex( index ).query( explicitRelationshipIndex( index ).query(
key, query instanceof Value ? ((Value) query).asObject() : query, source, target ) ); key, query instanceof Value ? ((Value) query).asObject() : query, source, target ) );
} }
Expand Down
Expand Up @@ -115,6 +115,7 @@ public void close()
relationship = NO_ID; relationship = NO_ID;
score = 0; score = 0;
expectedSize = 0; expectedSize = 0;
read = null;
} }


@Override @Override
Expand Down
Expand Up @@ -159,6 +159,7 @@ public void close()
edgePage = null; edgePage = null;
} }
current = null; current = null;
read = null;
setId( NO_ID ); setId( NO_ID );
clear(); clear();
} }
Expand Down
Expand Up @@ -111,6 +111,7 @@ public void close()
pageCursor.close(); pageCursor.close();
pageCursor = null; pageCursor = null;
} }
read = null;
reset(); reset();
} }


Expand Down
Expand Up @@ -425,6 +425,7 @@ public void close()
pageCursor.close(); pageCursor.close();
pageCursor = null; pageCursor = null;
} }
read = null;
reset(); reset();
} }


Expand Down
Expand Up @@ -343,8 +343,9 @@ private static class TestKernelTransaction extends KernelTransactionImplementati
mock( Pool.class ), Clocks.fakeClock(), CpuClock.NOT_AVAILABLE, HeapAllocation.NOT_AVAILABLE, mock( Pool.class ), Clocks.fakeClock(), CpuClock.NOT_AVAILABLE, HeapAllocation.NOT_AVAILABLE,
TransactionTracer.NULL, TransactionTracer.NULL,
LockTracer.NONE, PageCursorTracerSupplier.NULL, LockTracer.NONE, PageCursorTracerSupplier.NULL,
mock( StorageEngine.class, RETURNS_MOCKS ), new CanWrite(), mock(Cursors.class), AutoIndexing.UNSUPPORTED, mock( mock( StorageEngine.class, RETURNS_MOCKS ), new CanWrite(), mock( Cursors.class ),
ExplicitIndexStore.class) ); AutoIndexing.UNSUPPORTED, mock(
ExplicitIndexStore.class ) );


this.monitor = monitor; this.monitor = monitor;
} }
Expand Down
Expand Up @@ -190,7 +190,7 @@ public void shouldAcquireEntityWriteLockBeforeDeletingNode()
// GIVEN // GIVEN
when( nodeCursor.next() ).thenReturn( true ); when( nodeCursor.next() ).thenReturn( true );
when( nodeCursor.labels() ).thenReturn( LabelSet.NONE ); when( nodeCursor.labels() ).thenReturn( LabelSet.NONE );
when( allStoreHolder.nodeExists( 123 ) ).thenReturn( true ); when( allStoreHolder.nodeExistsInStore( 123 ) ).thenReturn( true );


// WHEN // WHEN
operations.nodeDelete( 123 ); operations.nodeDelete( 123 );
Expand Down

0 comments on commit 3d45cc2

Please sign in to comment.