Skip to content

Commit

Permalink
Fixes a recovery issue with legacy indexes
Browse files Browse the repository at this point in the history
Where updates could go to the wrong index. This was introduced by a recent
change where commits were batched during recovery. The core of the problem
was that index name ids are transaction-local, but was treated as global,
which became a problem for appliers that used the index name id as key to
redirect updates to the right indexes.

Also previously there were one applier created for every index, even
though legacy index appliers are designed to handle multiple indexes each.
This has been fixed.
  • Loading branch information
tinwelint committed May 12, 2015
1 parent 3177d80 commit f241d25
Show file tree
Hide file tree
Showing 10 changed files with 331 additions and 22 deletions.
1 change: 1 addition & 0 deletions .gitignore
Expand Up @@ -25,3 +25,4 @@ community/neo4j-harness/data
community/server-plugin-test/neo4j-home
enterprise/server-enterprise/neo4j-home
integrationtests/data
Thumbs.db
Expand Up @@ -20,6 +20,7 @@
package org.neo4j.kernel.impl.api;

import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

Expand All @@ -40,7 +41,14 @@
public class LegacyIndexApplier extends NeoCommandHandler.Adapter
{
private final LegacyIndexApplierLookup applierLookup;
private final Map<String,NeoCommandHandler> appliers = new HashMap<>();

// We have these two maps here for "applier lookup" performance reasons. Every command that we apply we must
// redirect to the correct applier, i.e. the _single_ applier for the provider managing the specific index.
// Looking up provider for an index has a certain cost so those are cached in applierByIndex.
private Map<String/*indexName*/,NeoCommandHandler> applierByNodeIndex = Collections.emptyMap();
private Map<String/*indexName*/,NeoCommandHandler> applierByRelationshipIndex = Collections.emptyMap();
private Map<String/*providerName*/,NeoCommandHandler> applierByProvider = Collections.emptyMap();

private final IndexConfigStore indexConfigStore;
private final IdOrderingQueue transactionOrdering;
private final long transactionId;
Expand All @@ -60,25 +68,69 @@ public LegacyIndexApplier( IndexConfigStore indexConfigStore, LegacyIndexApplier

private NeoCommandHandler applier( IndexCommand command ) throws IOException
{
byte nameId = command.getIndexNameId();
String indexName = defineCommand.getIndexName( nameId );
NeoCommandHandler applier = appliers.get( indexName );
// Have we got an applier for this index?
String indexName = defineCommand.getIndexName( command.getIndexNameId() );
Map<String,NeoCommandHandler> applierByIndex = applierByIndexMap( command );
NeoCommandHandler applier = applierByIndex.get( indexName );
if ( applier == null )
{
// We don't. Have we got an applier for the provider of this index?
IndexEntityType entityType = IndexEntityType.byId( command.getEntityType() );
Map<String,String> config = indexConfigStore.get( entityType.entityClass(), indexName );
if ( config == null )
{
// This provider doesn't even exist, return an EMPTY handler, i.e. ignore these changes.
// Could be that the index provider is temporarily unavailable?
return NeoCommandHandler.EMPTY;
}
String providerName = config.get( PROVIDER );
applier = applierLookup.newApplier( providerName, mode.needsIdempotencyChecks() );
applier.visitIndexDefineCommand( defineCommand );
appliers.put( indexName, applier );
applier = applierByProvider.get( providerName );
if ( applier == null )
{
// We don't, so create the applier
applier = applierLookup.newApplier( providerName, mode.needsIdempotencyChecks() );
applier.visitIndexDefineCommand( defineCommand );
applierByProvider.put( providerName, applier );
}

// Also cache this applier for this index
applierByIndex.put( indexName, applier );
}
return applier;
}

// Some lazy creation of Maps for holding appliers per provider and index
private Map<String,NeoCommandHandler> applierByIndexMap( IndexCommand command )
{
if ( command.getEntityType() == IndexEntityType.Node.id() )
{
if ( applierByNodeIndex.isEmpty() )
{
applierByNodeIndex = new HashMap<>();
lazyCreateApplierByprovider();
}
return applierByNodeIndex;
}
if ( command.getEntityType() == IndexEntityType.Relationship.id() )
{
if ( applierByRelationshipIndex.isEmpty() )
{
applierByRelationshipIndex = new HashMap<>();
lazyCreateApplierByprovider();
}
return applierByRelationshipIndex;
}
throw new UnsupportedOperationException( "Unknown entity type " + command.getEntityType() );
}

private void lazyCreateApplierByprovider()
{
if ( applierByProvider.isEmpty() )
{
applierByProvider = new HashMap<>();
}
}

@Override
public boolean visitIndexAddNodeCommand( AddNodeCommand command ) throws IOException
{
Expand Down Expand Up @@ -121,7 +173,7 @@ public boolean visitIndexDefineCommand( IndexDefineCommand command ) throws IOEx
@Override
public void apply()
{
for ( NeoCommandHandler applier : appliers.values() )
for ( NeoCommandHandler applier : applierByProvider.values() )
{
applier.apply();
}
Expand All @@ -132,7 +184,7 @@ public void close()
{
try
{
for ( NeoCommandHandler applier : appliers.values() )
for ( NeoCommandHandler applier : applierByProvider.values() )
{
applier.close();
}
Expand Down
Expand Up @@ -70,6 +70,7 @@ private class RecoveryCommandHandler extends NeoCommandHandler.Delegator
{
private final String name;
private int applyCount;
private boolean applied;

RecoveryCommandHandler( String name, NeoCommandHandler applier )
{
Expand All @@ -80,10 +81,10 @@ private class RecoveryCommandHandler extends NeoCommandHandler.Delegator
@Override
public void apply()
{
assert !applied;
if ( ++applyCount % batchSize == 0 )
{
applyForReal();
appliers.remove( name );
}
}

Expand All @@ -94,12 +95,17 @@ public void apply()
@Override
public void close()
{
super.close();
if ( applied )
{
super.close();
}
}

private void applyForReal()
{
super.apply();
appliers.remove( name );
applied = true;
}
}
}
Expand Up @@ -516,4 +516,10 @@ private void internalAdd( T entity, String key, Object value, Statement statemen
{
type.add( statement.dataWriteOperations(), name, type.id( entity ), key, value );
}

@Override
public String toString()
{
return "Index[" + type + ", " + name + "]";
}
}
@@ -0,0 +1,118 @@
/*
* Copyright (c) 2002-2015 "Neo Technology,"
* Network Engine for Objects in Lund AB [http://neotechnology.com]
*
* This file is part of Neo4j.
*
* Neo4j is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
package org.neo4j.kernel.impl.api;

import org.junit.Rule;
import org.junit.Test;

import java.io.File;
import java.util.Map;

import org.neo4j.graphdb.Node;
import org.neo4j.graphdb.Relationship;
import org.neo4j.graphdb.index.IndexManager;
import org.neo4j.graphdb.mockfs.EphemeralFileSystemAbstraction;
import org.neo4j.helpers.collection.MapUtil;
import org.neo4j.kernel.impl.index.IndexCommand.AddNodeCommand;
import org.neo4j.kernel.impl.index.IndexCommand.AddRelationshipCommand;
import org.neo4j.kernel.impl.index.IndexConfigStore;
import org.neo4j.kernel.impl.index.IndexDefineCommand;
import org.neo4j.kernel.impl.transaction.command.NeoCommandHandler;
import org.neo4j.kernel.lifecycle.LifeRule;
import org.neo4j.test.EphemeralFileSystemRule;

import static org.mockito.Matchers.anyBoolean;
import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.eq;
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.helpers.collection.MapUtil.stringMap;
import static org.neo4j.kernel.impl.api.TransactionApplicationMode.INTERNAL;
import static org.neo4j.kernel.impl.transaction.log.TransactionIdStore.BASE_TX_ID;
import static org.neo4j.kernel.impl.util.IdOrderingQueue.BYPASS;

public class LegacyIndexApplierTest
{
public final @Rule LifeRule life = new LifeRule( true );
public final @Rule EphemeralFileSystemRule fs = new EphemeralFileSystemRule();

@Test
public void shouldOnlyCreateOneApplierPerProvider() throws Exception
{
// GIVEN
Map<String,Byte> names = MapUtil.<String,Byte> genericMap( "first", (byte) 0, "second", (byte) 1 );
Map<String,Byte> keys = MapUtil.<String,Byte> genericMap( "key", (byte) 0 );
String applierName = "test-applier";
IndexConfigStore config = newIndexConfigStore( names, applierName );
LegacyIndexApplierLookup applierLookup = mock( LegacyIndexApplierLookup.class );
when( applierLookup.newApplier( anyString(), anyBoolean() ) ).thenReturn( mock( NeoCommandHandler.class ) );
try ( LegacyIndexApplier applier = new LegacyIndexApplier( config, applierLookup, BYPASS, BASE_TX_ID, INTERNAL ) )
{
// WHEN
IndexDefineCommand definitions = definitions( names, keys );
applier.visitIndexDefineCommand( definitions );
applier.visitIndexAddNodeCommand( addNodeToIndex( definitions, "first" ) );
applier.visitIndexAddNodeCommand( addNodeToIndex( definitions, "second" ) );
applier.visitIndexAddRelationshipCommand( addRelationshipToIndex( definitions, "second" ) );
applier.apply();
}

// THEN
verify( applierLookup, times( 1 ) ).newApplier( eq( applierName ), anyBoolean() );
}

private static AddRelationshipCommand addRelationshipToIndex( IndexDefineCommand definitions, String indexName )
{
AddRelationshipCommand command = new AddRelationshipCommand();
command.init( definitions.getOrAssignIndexNameId( indexName ), 0L, (byte) 0, null, 1, 2 );
return command;
}

private static AddNodeCommand addNodeToIndex( IndexDefineCommand definitions, String indexName )
{
AddNodeCommand command = new AddNodeCommand();
command.init( definitions.getOrAssignIndexNameId( indexName ), 0L, (byte) 0, null );
return command;
}

private static IndexDefineCommand definitions( Map<String,Byte> names, Map<String,Byte> keys )
{
IndexDefineCommand definitions = new IndexDefineCommand();
definitions.init( names, keys );
return definitions;
}

private IndexConfigStore newIndexConfigStore( Map<String,Byte> names, String providerName )
{
File dir = new File( "conf" );
EphemeralFileSystemAbstraction fileSystem = fs.get();
fileSystem.mkdirs( dir );
IndexConfigStore store = life.add( new IndexConfigStore( dir, fileSystem ) );
for ( Map.Entry<String,Byte> name : names.entrySet() )
{
store.set( Node.class, name.getKey(), stringMap( IndexManager.PROVIDER, providerName ) );
store.set( Relationship.class, name.getKey(), stringMap( IndexManager.PROVIDER, providerName ) );
}
return store;
}
}
Expand Up @@ -25,6 +25,18 @@

public class LifeRule extends LifeSupport implements TestRule
{
private final boolean autoStart;

public LifeRule()
{
this( false );
}

public LifeRule( boolean autoStart )
{
this.autoStart = autoStart;
}

@Override
public Statement apply( final Statement base, Description description )
{
Expand All @@ -35,6 +47,10 @@ public void evaluate() throws Throwable
{
try
{
if ( autoStart )
{
start();
}
base.evaluate();
}
catch ( Throwable failure )
Expand Down
Expand Up @@ -110,7 +110,7 @@ private void applyDocuments( IndexWriter writer, IndexType type,
public void close() throws IOException
{
applyDocuments( writer, indexType, documents );
if ( writer != null && !recovery )
if ( writer != null )
{
dataSource.invalidateIndexSearcher( identifier );
}
Expand All @@ -132,5 +132,11 @@ static class DocumentContext
this.exists = exists;
this.entityId = entityId;
}

@Override
public String toString()
{
return "DocumentContext[document=" + document + ", exists=" + exists + ", entityId=" + entityId + "]";
}
}
}

0 comments on commit f241d25

Please sign in to comment.