Skip to content

Commit

Permalink
Legacy indexes can be read when in read_only mode
Browse files Browse the repository at this point in the history
Previously simply getting a handle to a legacy index would have the transaction
think that it needed write access, by mistake. This is now changed so that
first the existence of the index is checked with only read requirement and then
if it doesn't gets escalated to create-and-get it.
  • Loading branch information
tinwelint committed Aug 9, 2017
1 parent ed11cca commit dfb34ce
Show file tree
Hide file tree
Showing 12 changed files with 332 additions and 22 deletions.
Expand Up @@ -377,6 +377,24 @@ IndexDescriptor uniqueIndexGetForLabelAndPropertyKey( int labelId, int propertyK
//== LEGACY INDEX OPERATIONS ================ //== LEGACY INDEX OPERATIONS ================
//=========================================== //===========================================


/**
* @param indexName name of node index to check for existence.
* @param customConfiguration if {@code null} the configuration of existing won't be matched, otherwise it will
* be matched and a mismatch will throw {@link IllegalArgumentException}.
* @return whether or not node legacy index with name {@code indexName} exists.
* @throws IllegalArgumentException on index existence with provided mismatching {@code customConfiguration}.
*/
boolean nodeLegacyIndexExists( String indexName, Map<String,String> customConfiguration );

/**
* @param indexName name of relationship index to check for existence.
* @param customConfiguration if {@code null} the configuration of existing won't be matched, otherwise it will
* be matched and a mismatch will throw {@link IllegalArgumentException}.
* @return whether or not relationship legacy index with name {@code indexName} exists.
* @throws IllegalArgumentException on index existence with provided mismatching {@code customConfiguration}.
*/
boolean relationshipLegacyIndexExists( String indexName, Map<String,String> customConfiguration );

Map<String, String> nodeLegacyIndexGetConfiguration( String indexName ) Map<String, String> nodeLegacyIndexGetConfiguration( String indexName )
throws LegacyIndexNotFoundKernelException; throws LegacyIndexNotFoundKernelException;


Expand Down
Expand Up @@ -38,7 +38,27 @@ public interface LegacyIndexTransactionState extends RecordState


LegacyIndex relationshipChanges( String indexName ) throws LegacyIndexNotFoundKernelException; LegacyIndex relationshipChanges( String indexName ) throws LegacyIndexNotFoundKernelException;


void createIndex( IndexEntityType node, String name, Map<String, String> config ); void createIndex( IndexEntityType entityType, String indexName, Map<String, String> config );


void deleteIndex( IndexEntityType entityType, String indexName ); void deleteIndex( IndexEntityType entityType, String indexName );

/**
* Checks whether or not index with specific {@code name} exists.
* Optionally the specific {@code config} is verified to be matching.
*
* This method can either return {@code boolean} or {@code throw} exception on:
* <ul>
* <li>index exists, config is provided and matching => {@code true}</li>
* <li>index exists, config is provided and NOT matching => {@code throw exception}</li>
* <li>index exists, config is NOT provided => {@code true}</li>
* <li>index does NOT exist => {@code false}</li>
* </ul>
*
* @param entityType {@link IndexEntityType} for the index.
* @param indexName name of the index.
* @param config configuration which must match the existing index, if it exists. {@code null} means
* that the configuration doesn't need to be checked.
* @return {@code true} if the index with the specific {@code name} and {@code entityType} exists, otherwise {@code false}.
*/
boolean checkIndexExistence( IndexEntityType entityType, String indexName, Map<String, String> config );
} }
Expand Up @@ -94,4 +94,10 @@ public void extractCommands( Collection<StorageCommand> target ) throws Transact
{ {
txState.extractCommands( target ); txState.extractCommands( target );
} }

@Override
public boolean checkIndexExistence( IndexEntityType entityType, String indexName, Map<String,String> config )
{
return txState.checkIndexExistence( entityType, indexName, config );
}
} }
Expand Up @@ -1182,6 +1182,20 @@ public void releaseShared( ResourceType type, long id )
// </Locking> // </Locking>


// <Legacy index> // <Legacy index>
@Override
public boolean nodeLegacyIndexExists( String indexName, Map<String,String> customConfiguration )
{
statement.assertOpen();
return legacyIndexRead().nodeLegacyIndexExists( statement, indexName, customConfiguration );
}

@Override
public boolean relationshipLegacyIndexExists( String indexName, Map<String,String> customConfiguration )
{
statement.assertOpen();
return legacyIndexRead().relationshipLegacyIndexExists( statement, indexName, customConfiguration );
}

@Override @Override
public LegacyIndexHits nodeLegacyIndexGet( String indexName, String key, Object value ) public LegacyIndexHits nodeLegacyIndexGet( String indexName, String key, Object value )
throws LegacyIndexNotFoundKernelException throws LegacyIndexNotFoundKernelException
Expand Down
Expand Up @@ -1394,7 +1394,6 @@ public int relationshipTypeCount( KernelStatement statement )
return storeLayer.relationshipTypeCount(); return storeLayer.relationshipTypeCount();
} }


// <Legacy index>
@Override @Override
public <EXCEPTION extends Exception> void relationshipVisit( KernelStatement statement, public <EXCEPTION extends Exception> void relationshipVisit( KernelStatement statement,
long relId, RelationshipVisitor<EXCEPTION> visitor ) throws EntityNotFoundException, EXCEPTION long relId, RelationshipVisitor<EXCEPTION> visitor ) throws EntityNotFoundException, EXCEPTION
Expand All @@ -1409,6 +1408,19 @@ public <EXCEPTION extends Exception> void relationshipVisit( KernelStatement sta
storeLayer.relationshipVisit( relId, visitor ); storeLayer.relationshipVisit( relId, visitor );
} }


// <Legacy index>
@Override
public boolean nodeLegacyIndexExists( KernelStatement statement, String indexName, Map<String,String> customConfiguration )
{
return statement.legacyIndexTxState().checkIndexExistence( IndexEntityType.Node, indexName, customConfiguration );
}

@Override
public boolean relationshipLegacyIndexExists( KernelStatement statement, String indexName, Map<String,String> customConfiguration )
{
return statement.legacyIndexTxState().checkIndexExistence( IndexEntityType.Relationship, indexName, customConfiguration );
}

@Override @Override
public LegacyIndexHits nodeLegacyIndexGet( KernelStatement statement, String indexName, String key, Object value ) public LegacyIndexHits nodeLegacyIndexGet( KernelStatement statement, String indexName, String key, Object value )
throws LegacyIndexNotFoundKernelException throws LegacyIndexNotFoundKernelException
Expand Down
Expand Up @@ -27,6 +27,26 @@


public interface LegacyIndexReadOperations public interface LegacyIndexReadOperations
{ {
/**
* @param statement {@link KernelStatement} to use for state.
* @param indexName name of node index to check for existence.
* @param customConfiguration if {@code null} the configuration of existing won't be matched, otherwise it will
* be matched and a mismatch will throw {@link IllegalArgumentException}.
* @return whether or not node legacy index with name {@code indexName} exists.
* @throws IllegalArgumentException on index existence with provided mismatching {@code customConfiguration}.
*/
boolean nodeLegacyIndexExists( KernelStatement statement, String indexName, Map<String,String> customConfiguration );

/**
* @param statement {@link KernelStatement} to use for state.
* @param indexName name of relationship index to check for existence.
* @param customConfiguration if {@code null} the configuration of existing won't be matched, otherwise it will
* be matched and a mismatch will throw {@link IllegalArgumentException}.
* @return whether or not relationship legacy index with name {@code indexName} exists.
* @throws IllegalArgumentException on index existence with provided mismatching {@code customConfiguration}.
*/
boolean relationshipLegacyIndexExists( KernelStatement statement, String indexName, Map<String,String> customConfiguration );

Map<String, String> nodeLegacyIndexGetConfiguration( KernelStatement statement, String indexName ) Map<String, String> nodeLegacyIndexGetConfiguration( KernelStatement statement, String indexName )
throws LegacyIndexNotFoundKernelException; throws LegacyIndexNotFoundKernelException;


Expand Down
Expand Up @@ -47,6 +47,8 @@
import org.neo4j.kernel.spi.legacyindex.LegacyIndexProviderTransaction; import org.neo4j.kernel.spi.legacyindex.LegacyIndexProviderTransaction;
import org.neo4j.storageengine.api.StorageCommand; import org.neo4j.storageengine.api.StorageCommand;


import static org.neo4j.kernel.impl.index.LegacyIndexStore.assertConfigMatches;

/** /**
* Provides access to {@link LegacyIndex indexes}. Holds transaction state for all providers in a transaction. * Provides access to {@link LegacyIndex indexes}. Holds transaction state for all providers in a transaction.
* A equivalent to TransactionRecordState, but for legacy indexes. * A equivalent to TransactionRecordState, but for legacy indexes.
Expand Down Expand Up @@ -241,4 +243,19 @@ public boolean hasChanges()
{ {
return defineCommand != null; return defineCommand != null;
} }

@Override
public boolean checkIndexExistence( IndexEntityType entityType, String indexName, Map<String,String> config )
{
Map<String, String> configuration = indexConfigStore.get( entityType.entityClass(), indexName );
if ( configuration == null )
{
return false;
}

String providerName = configuration.get( IndexManager.PROVIDER );
IndexImplementation provider = providerLookup.apply( providerName );
assertConfigMatches( provider, indexName, configuration, config );
return true;
}
} }
Expand Up @@ -46,10 +46,13 @@ public Index<Node> getOrCreateNodeIndex( String indexName, Map<String,String> cu
{ {
try ( Statement statement = transactionBridge.get() ) try ( Statement statement = transactionBridge.get() )
{ {
// There's a sub-o-meta thing here where we create index config, if ( !statement.readOperations().nodeLegacyIndexExists( indexName, customConfiguration ) )
// and the index will itself share the same IndexConfigStore as us and pick up and use {
// that. We should pass along config somehow with calls. // There's a sub-o-meta thing here where we create index config,
statement.dataWriteOperations().nodeLegacyIndexCreateLazily( indexName, customConfiguration ); // and the index will itself share the same IndexConfigStore as us and pick up and use
// that. We should pass along config somehow with calls.
statement.dataWriteOperations().nodeLegacyIndexCreateLazily( indexName, customConfiguration );
}
return new LegacyIndexProxy<>( indexName, LegacyIndexProxy.Type.NODE, gds, transactionBridge ); return new LegacyIndexProxy<>( indexName, LegacyIndexProxy.Type.NODE, gds, transactionBridge );
} }
catch ( InvalidTransactionTypeKernelException e ) catch ( InvalidTransactionTypeKernelException e )
Expand All @@ -64,10 +67,13 @@ public RelationshipIndex getOrCreateRelationshipIndex( String indexName, Map<Str
{ {
try ( Statement statement = transactionBridge.get() ) try ( Statement statement = transactionBridge.get() )
{ {
// There's a sub-o-meta thing here where we create index config, if ( !statement.readOperations().relationshipLegacyIndexExists( indexName, customConfiguration ) )
// and the index will itself share the same IndexConfigStore as us and pick up and use {
// that. We should pass along config somehow with calls. // There's a sub-o-meta thing here where we create index config,
statement.dataWriteOperations().relationshipLegacyIndexCreateLazily( indexName, customConfiguration ); // and the index will itself share the same IndexConfigStore as us and pick up and use
// that. We should pass along config somehow with calls.
statement.dataWriteOperations().relationshipLegacyIndexCreateLazily( indexName, customConfiguration );
}
return new RelationshipLegacyIndexProxy( indexName, gds, transactionBridge ); return new RelationshipLegacyIndexProxy( indexName, gds, transactionBridge );
} }
catch ( InvalidTransactionTypeKernelException e ) catch ( InvalidTransactionTypeKernelException e )
Expand Down
Expand Up @@ -122,7 +122,7 @@ private Map<String, String> findIndexConfig(
return Collections.unmodifiableMap( configToUse ); return Collections.unmodifiableMap( configToUse );
} }


private void assertConfigMatches( IndexImplementation indexProvider, String indexName, public static void assertConfigMatches( IndexImplementation indexProvider, String indexName,
Map<String, String> storedConfig, Map<String, String> suppliedConfig ) Map<String, String> storedConfig, Map<String, String> suppliedConfig )
{ {
if ( suppliedConfig != null && !indexProvider.configMatches( storedConfig, suppliedConfig ) ) if ( suppliedConfig != null && !indexProvider.configMatches( storedConfig, suppliedConfig ) )
Expand Down
Expand Up @@ -23,6 +23,7 @@


import java.util.Arrays; import java.util.Arrays;
import java.util.HashSet; import java.util.HashSet;
import java.util.Map;
import java.util.Set; import java.util.Set;
import java.util.function.Function; import java.util.function.Function;


Expand All @@ -39,13 +40,23 @@


import static java.util.Collections.singletonMap; import static java.util.Collections.singletonMap;
import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyMap;
import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.eq; import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when; import static org.mockito.Mockito.when;


public class LegacyIndexTransactionStateImplTest public class LegacyIndexTransactionStateImplTest
{ {
private final Map<String,String> config = singletonMap( IndexManager.PROVIDER, "test" );
private final IndexImplementation provider = mock( IndexImplementation.class );
private Function<String,IndexImplementation> providerLookup;
private IndexConfigStore indexConfigStore;

@Test @Test
public void tracksNodeCommands() public void tracksNodeCommands()
{ {
Expand Down Expand Up @@ -194,6 +205,77 @@ public void removalOfRelationshipIndexDoesNotClearNodeCommandsForNodeIndexWithSa
assertEquals( expectedCommands, extractCommands( state ) ); assertEquals( expectedCommands, extractCommands( state ) );
} }


@Test
public void shouldReportIndexExists() throws Exception
{
// given
LegacyIndexTransactionStateImpl state = newLegacyIndexTxState();

// when
boolean nodeExists = state.checkIndexExistence( IndexEntityType.Node, "name", null );
boolean relExists = state.checkIndexExistence( IndexEntityType.Relationship, "name", null );

// then
assertTrue( nodeExists );
assertTrue( relExists );
}

@Test
public void shouldReportIndexExistsWithMatchingConfiguration() throws Exception
{
// given
LegacyIndexTransactionStateImpl state = newLegacyIndexTxState();
when( provider.configMatches( anyMap(), anyMap() ) ).thenReturn( true );

// when
boolean nodeExists = state.checkIndexExistence( IndexEntityType.Node, "name", config );
boolean relExists = state.checkIndexExistence( IndexEntityType.Node, "name", config );

// then
assertTrue( nodeExists );
assertTrue( relExists );
}

@Test
public void shouldThrowOnIndexExistsWithMismatchingConfiguration() throws Exception
{
// given
LegacyIndexTransactionStateImpl state = newLegacyIndexTxState();
when( provider.configMatches( anyMap(), anyMap() ) ).thenReturn( false );

// when
try
{
state.checkIndexExistence( IndexEntityType.Node, "name", config );
fail( "Should've failed" );
}
catch ( IllegalArgumentException e )
{ // then good
}
try
{
state.checkIndexExistence( IndexEntityType.Node, "name", config );
fail( "Should have failed" );
}
catch ( IllegalArgumentException e )
{ // then good
}
}

@Test
public void shouldReportIndexDoesNotExist() throws Exception
{
// given
LegacyIndexTransactionStateImpl state = newLegacyIndexTxState();
when( indexConfigStore.get( any( Class.class ), anyString() ) ).thenReturn( null );

// when
boolean exists = state.checkIndexExistence( IndexEntityType.Relationship, "name", null );

// then
assertFalse( exists );
}

private static Set<StorageCommand> extractCommands( LegacyIndexTransactionStateImpl state ) private static Set<StorageCommand> extractCommands( LegacyIndexTransactionStateImpl state )
{ {
Set<StorageCommand> commands = new HashSet<>(); Set<StorageCommand> commands = new HashSet<>();
Expand Down Expand Up @@ -229,15 +311,13 @@ private static Command removeRelationship( int index, long id, int key, Object v
return command; return command;
} }


private static LegacyIndexTransactionStateImpl newLegacyIndexTxState() private LegacyIndexTransactionStateImpl newLegacyIndexTxState()
{ {
IndexConfigStore indexConfigStore = mock( IndexConfigStore.class ); indexConfigStore = mock( IndexConfigStore.class );
when( indexConfigStore.get( eq( Node.class ), anyString() ) ) when( indexConfigStore.get( eq( Node.class ), anyString() ) ).thenReturn( config );
.thenReturn( singletonMap( IndexManager.PROVIDER, "test" ) ); when( indexConfigStore.get( eq( Relationship.class ), anyString() ) ).thenReturn( config );
when( indexConfigStore.get( eq( Relationship.class ), anyString() ) )
.thenReturn( singletonMap( IndexManager.PROVIDER, "test" ) );


Function<String,IndexImplementation> providerLookup = s -> mock( IndexImplementation.class ); providerLookup = s -> provider;


return new LegacyIndexTransactionStateImpl( indexConfigStore, providerLookup ); return new LegacyIndexTransactionStateImpl( indexConfigStore, providerLookup );
} }
Expand Down
Expand Up @@ -61,6 +61,8 @@
import org.neo4j.kernel.impl.store.StoreId; import org.neo4j.kernel.impl.store.StoreId;
import org.neo4j.kernel.internal.GraphDatabaseAPI; import org.neo4j.kernel.internal.GraphDatabaseAPI;


import static org.neo4j.helpers.collection.MapUtil.stringMap;

public abstract class DatabaseRule extends ExternalResource implements GraphDatabaseAPI public abstract class DatabaseRule extends ExternalResource implements GraphDatabaseAPI
{ {
private GraphDatabaseBuilder databaseBuilder; private GraphDatabaseBuilder databaseBuilder;
Expand Down Expand Up @@ -327,10 +329,11 @@ public GraphDatabaseAPI getGraphDatabaseAPI()
return database; return database;
} }


public synchronized void ensureStarted() public synchronized void ensureStarted( String... additionalConfig )
{ {
if ( database == null ) if ( database == null )
{ {
applyConfigChanges( additionalConfig );
database = (GraphDatabaseAPI) databaseBuilder.newGraphDatabase(); database = (GraphDatabaseAPI) databaseBuilder.newGraphDatabase();
storeDir = database.getStoreDir(); storeDir = database.getStoreDir();
statementSupplier = resolveDependency( ThreadToStatementContextBridge.class ); statementSupplier = resolveDependency( ThreadToStatementContextBridge.class );
Expand Down Expand Up @@ -366,20 +369,26 @@ public interface RestartAction
}; };
} }


public GraphDatabaseAPI restartDatabase() throws IOException public GraphDatabaseAPI restartDatabase( String... configChanges ) throws IOException
{ {
return restartDatabase( RestartAction.EMPTY ); return restartDatabase( RestartAction.EMPTY, configChanges );
} }


public GraphDatabaseAPI restartDatabase( RestartAction action ) throws IOException public GraphDatabaseAPI restartDatabase( RestartAction action, String... configChanges ) throws IOException
{ {
FileSystemAbstraction fs = resolveDependency( FileSystemAbstraction.class ); FileSystemAbstraction fs = resolveDependency( FileSystemAbstraction.class );
database.shutdown(); database.shutdown();
action.run( fs, new File( storeDir ) ); action.run( fs, new File( storeDir ) );
database = null; database = null;
applyConfigChanges( configChanges );
return getGraphDatabaseAPI(); return getGraphDatabaseAPI();
} }


private void applyConfigChanges( String[] configChanges )
{
databaseBuilder.setConfig( stringMap( configChanges ) );
}

@Override @Override
public void shutdown() public void shutdown()
{ {
Expand Down

0 comments on commit dfb34ce

Please sign in to comment.