Skip to content

Commit

Permalink
GBPTree.readHeader don't need layout
Browse files Browse the repository at this point in the history 
Layout was just needed to read header as an extra safety net.
We verified that the index that you read the header from was actually
the index that you thought it was, by verifying the layout. This created
more problems than it solved and we can drive without this seat belt
holding us back.

Also aligning exception type on missing meta page and missing state page.
Use MetadataMismatchException in both cases.
  • Loading branch information
@burqen
burqen committed Mar 16, 2018
1 parent a1071de commit a46adca
Show file tree
Hide file tree
Showing 8 changed files with 39 additions and 47 deletions.
17 changes: 8 additions & 9 deletions community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTree.java
View file
Expand Up @@ -580,18 +580,16 @@ private void loadState( PagedFile pagedFile, Header.Reader headerReader ) throws
*
* @param pageCache {@link PageCache} to use to map index file
* @param indexFile {@link File} containing the actual index
* @param layout {@link Layout} only used to verify compatibility with stored identifier and version. Can be 'dummy' implementation.
* @param headerReader reads header data, previously written using {@link #checkpoint(IOLimiter, Consumer)}
* or {@link #close()}
* @throws IOException On page cache error
* @throws MetadataMismatchException if metadata does match given parameters
* @throws MetadataMismatchException if some meta page is missing (tree not fully initialized)
*/
public static void readHeader( PageCache pageCache, File indexFile, Layout<?,?> layout, Header.Reader headerReader )
public static void readHeader( PageCache pageCache, File indexFile, Header.Reader headerReader )
throws IOException, MetadataMismatchException
{
try ( PagedFile pagedFile = openExistingIndexFile( pageCache, indexFile ) )
{
readMeta( layout, pagedFile ).verify( layout );
Pair<TreeState,TreeState> states = loadStatePages( pagedFile );
TreeState state = TreeStatePair.selectNewestValidState( states );
try ( PageCursor cursor = pagedFile.io( state.pageId(), PagedFile.PF_SHARED_READ_LOCK ) )
Expand All @@ -605,7 +603,7 @@ public static void readHeader( PageCache pageCache, File indexFile, Layout<?,?>
// Decorate outgoing exceptions with basic tree information. This is similar to how the constructor
// appends its information, but the constructor has read more information at that point so this one
// is a bit more sparse on information.
withMessage( t, t.getMessage() + " | " + format( "GBPTree[file:%s, layout:%s]", indexFile, layout ) );
withMessage( t, t.getMessage() + " | " + format( "GBPTree[file:%s]", indexFile ) );
throw t;
}
}
Expand Down Expand Up @@ -696,22 +694,23 @@ private static TreeState other( Pair<TreeState,TreeState> states, TreeState stat
*
* @param pagedFile {@link PagedFile} to read the state pages from.
* @return both read state pages.
* @throws IOException if state pages are missing (file is smaller than that) or if they are both empty.
* @throws MetadataMismatchException if state pages are missing (file is smaller than that) or if they are both empty.
* @throws IOException on {@link PageCursor} error.
*/
private static Pair<TreeState,TreeState> loadStatePages( PagedFile pagedFile ) throws IOException
private static Pair<TreeState,TreeState> loadStatePages( PagedFile pagedFile ) throws MetadataMismatchException, IOException
{
try
{
Pair<TreeState,TreeState> states = readStatePages( pagedFile );
if ( states.getLeft().isEmpty() && states.getRight().isEmpty() )
{
throw new IOException( "Index is not fully initialized since its state pages are empty" );
throw new MetadataMismatchException( "Index is not fully initialized since its state pages are empty" );
}
return states;
}
catch ( IllegalStateException e )
{
throw new IOException( "Index is not fully initialized since it's missing state pages", e );
throw new MetadataMismatchException( "Index is not fully initialized since it's missing state pages", e );
}
}

Expand Down
6 changes: 0 additions & 6 deletions community/index/src/main/java/org/neo4j/index/internal/gbptree/Layout.java
View file
Expand Up @@ -19,10 +19,8 @@
*/
package org.neo4j.index.internal.gbptree;

import java.io.File;
import java.util.Comparator;

import org.neo4j.io.pagecache.PageCache;
import org.neo4j.io.pagecache.PageCursor;

import static java.lang.String.format;
Expand Down Expand Up @@ -187,10 +185,6 @@ static long namedIdentifier( String name, int identifier )
* <p>
* When opening a {@link GBPTree tree} to 'use' it, read and write to it, providing a layout with the right compatibility is
* important because it decides how to read and write entries in the tree.
* A layout also needs to be provided when only {@link GBPTree#readHeader(PageCache, File, Layout, Header.Reader)} reading header}
* of tree. In this case there is no intention of reading or writing entries in the tree. If multiple layout implementations share
* the same header layout (but have different structure for entries) then a layout that is compatible with either of those layouts
* can be provided for the read header operation.
*
* @param layoutIdentifier the stored layout identifier we want to check compatibility against.
* @param majorVersion the stored major version we want to check compatibility against.
Expand Down
6 changes: 3 additions & 3 deletions ...nity/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreePartialCreateFuzzIT.java
View file
Expand Up @@ -75,9 +75,9 @@ public void shouldDetectAndThrowIOExceptionOnPartiallyCreatedFile() throws Excep
// check readHeader
try
{
GBPTree.readHeader( pageCache, file, layout, NO_HEADER_READER );
GBPTree.readHeader( pageCache, file, NO_HEADER_READER );
}
catch ( IOException e )
catch ( MetadataMismatchException | IOException e )
{
// It's OK if the process was destroyed
assertNotEquals( 0, exitCode );
Expand All @@ -88,7 +88,7 @@ public void shouldDetectAndThrowIOExceptionOnPartiallyCreatedFile() throws Excep
{
new GBPTreeBuilder<>( pageCache, file, layout ).build().close();
}
catch ( IOException e )
catch ( MetadataMismatchException | IOException e )
{
// It's OK if the process was destroyed
assertNotEquals( 0, exitCode );
Expand Down
34 changes: 17 additions & 17 deletions community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeTest.java
View file
Expand Up @@ -641,7 +641,7 @@ private void verifyHeader( PageCache pageCache, byte[] expectedHeader ) throws I

// WHEN
// Read separate
GBPTree.readHeader( pageCache, indexFile, layout, headerReader );
GBPTree.readHeader( pageCache, indexFile, headerReader );

assertEquals( expectedHeader.length, length.get() );
assertArrayEquals( expectedHeader, readHeader );
Expand All @@ -654,7 +654,7 @@ public void readHeaderMustThrowIfFileDoesNotExist() throws Exception
File doesNotExist = new File( "Does not exist" );
try
{
GBPTree.readHeader( createPageCache( DEFAULT_PAGE_SIZE ), doesNotExist, layout, NO_HEADER_READER );
GBPTree.readHeader( createPageCache( DEFAULT_PAGE_SIZE ), doesNotExist, NO_HEADER_READER );
fail( "Should have failed" );
}
catch ( NoSuchFileException e )
Expand All @@ -666,7 +666,7 @@ public void readHeaderMustThrowIfFileDoesNotExist() throws Exception
@Test
public void openWithReadHeaderMustThrowMetadataMismatchExceptionIfFileIsEmpty() throws Exception
{
openMustThrowMetadataMismatchExceptionIfFileIsEmpty( pageCache -> GBPTree.readHeader( pageCache, indexFile, layout, NO_HEADER_READER ) );
openMustThrowMetadataMismatchExceptionIfFileIsEmpty( pageCache -> GBPTree.readHeader( pageCache, indexFile, NO_HEADER_READER ) );
}

@Test
Expand Down Expand Up @@ -694,19 +694,19 @@ private void openMustThrowMetadataMismatchExceptionIfFileIsEmpty( ThrowingConsum
}

@Test
public void readHeaderMustThrowIOExceptionIfSomeMetaPageIsMissing() throws Exception
public void readHeaderMustThrowMetadataMismatchExceptionIfSomeMetaPageIsMissing() throws Exception
{
openMustThrowIOExceptionIfSomeMetaPageIsMissing(
pageCache -> GBPTree.readHeader( pageCache, indexFile, layout, NO_HEADER_READER ) );
openMustThrowMetadataMismatchExceptionIfSomeMetaPageIsMissing(
pageCache -> GBPTree.readHeader( pageCache, indexFile, NO_HEADER_READER ) );
}

@Test
public void constructorMustThrowIOExceptionIfSomeMetaPageIsMissing() throws Exception
public void constructorMustThrowMetadataMismatchExceptionIfSomeMetaPageIsMissing() throws Exception
{
openMustThrowIOExceptionIfSomeMetaPageIsMissing( pageCache -> index( pageCache ).build() );
openMustThrowMetadataMismatchExceptionIfSomeMetaPageIsMissing( pageCache -> index( pageCache ).build() );
}

private void openMustThrowIOExceptionIfSomeMetaPageIsMissing( ThrowingConsumer<PageCache,IOException> opener ) throws Exception
private void openMustThrowMetadataMismatchExceptionIfSomeMetaPageIsMissing( ThrowingConsumer<PageCache,IOException> opener ) throws Exception
{
// given an existing index with only the first page in it
PageCache pageCache = createPageCache( DEFAULT_PAGE_SIZE );
Expand All @@ -721,7 +721,7 @@ private void openMustThrowIOExceptionIfSomeMetaPageIsMissing( ThrowingConsumer<P
opener.accept( pageCache );
fail( "Should've thrown IOException" );
}
catch ( IOException e )
catch ( MetadataMismatchException e )
{
// then good
}
Expand All @@ -730,17 +730,17 @@ private void openMustThrowIOExceptionIfSomeMetaPageIsMissing( ThrowingConsumer<P
@Test
public void readHeaderMustThrowIOExceptionIfStatePagesAreAllZeros() throws Exception
{
openMustThrowIOExceptionIfStatePagesAreAllZeros(
pageCache -> GBPTree.readHeader( pageCache, indexFile, layout, NO_HEADER_READER ) );
openMustThrowMetadataMismatchExceptionIfStatePagesAreAllZeros(
pageCache -> GBPTree.readHeader( pageCache, indexFile, NO_HEADER_READER ) );
}

@Test
public void constructorMustThrowIOExceptionIfStatePagesAreAllZeros() throws Exception
public void constructorMustThrowMetadataMismatchExceptionIfStatePagesAreAllZeros() throws Exception
{
openMustThrowIOExceptionIfStatePagesAreAllZeros( pageCache -> index( pageCache ).build() );
openMustThrowMetadataMismatchExceptionIfStatePagesAreAllZeros( pageCache -> index( pageCache ).build() );
}

private void openMustThrowIOExceptionIfStatePagesAreAllZeros( ThrowingConsumer<PageCache,IOException> opener ) throws Exception
private void openMustThrowMetadataMismatchExceptionIfStatePagesAreAllZeros( ThrowingConsumer<PageCache,IOException> opener ) throws Exception
{
// given an existing index with all-zero state pages
PageCache pageCache = createPageCache( DEFAULT_PAGE_SIZE );
Expand All @@ -761,7 +761,7 @@ private void openMustThrowIOExceptionIfStatePagesAreAllZeros( ThrowingConsumer<P
opener.accept( pageCache );
fail( "Should've thrown IOException" );
}
catch ( IOException e )
catch ( MetadataMismatchException e )
{
// then good
}
Expand All @@ -786,7 +786,7 @@ public void readHeaderMustWorkWithOpenIndex() throws Exception
length.set( headerData.limit() );
headerData.get( readHeader );
};
GBPTree.readHeader( pageCache, indexFile, layout, headerReader );
GBPTree.readHeader( pageCache, indexFile, headerReader );

// THEN
assertEquals( headerBytes.length, length.get() );
Expand Down
4 changes: 2 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeIndexProvider.java
View file
Expand Up @@ -94,7 +94,7 @@ public String getPopulationFailure( long indexId, SchemaIndexDescriptor descript
{
try
{
String failureMessage = NativeSchemaIndexes.readFailureMessage( pageCache, nativeIndexFileFromIndexId( indexId ), layout( descriptor ) );
String failureMessage = NativeSchemaIndexes.readFailureMessage( pageCache, nativeIndexFileFromIndexId( indexId ) );
if ( failureMessage == null )
{
throw new IllegalStateException( "Index " + indexId + " isn't failed" );
Expand All @@ -112,7 +112,7 @@ public InternalIndexState getInitialState( long indexId, SchemaIndexDescriptor d
{
try
{
return NativeSchemaIndexes.readState( pageCache, nativeIndexFileFromIndexId( indexId ), layout( descriptor ) );
return NativeSchemaIndexes.readState( pageCache, nativeIndexFileFromIndexId( indexId ) );
}
catch ( IOException e )
{
Expand Down
9 changes: 4 additions & 5 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaIndexes.java
View file
Expand Up @@ -23,7 +23,6 @@
import java.io.IOException;

import org.neo4j.index.internal.gbptree.GBPTree;
import org.neo4j.index.internal.gbptree.Layout;
import org.neo4j.internal.kernel.api.InternalIndexState;
import org.neo4j.io.pagecache.PageCache;

Expand All @@ -36,10 +35,10 @@ public class NativeSchemaIndexes
private NativeSchemaIndexes()
{}

public static InternalIndexState readState( PageCache pageCache, File indexFile, Layout<?,?> layout ) throws IOException
public static InternalIndexState readState( PageCache pageCache, File indexFile ) throws IOException
{
NativeSchemaIndexHeaderReader headerReader = new NativeSchemaIndexHeaderReader();
GBPTree.readHeader( pageCache, indexFile, layout, headerReader );
GBPTree.readHeader( pageCache, indexFile, headerReader );
switch ( headerReader.state )
{
case BYTE_FAILED:
Expand All @@ -53,11 +52,11 @@ public static InternalIndexState readState( PageCache pageCache, File indexFile,
}
}

public static String readFailureMessage( PageCache pageCache, File indexFile, Layout<?,?> layout )
static String readFailureMessage( PageCache pageCache, File indexFile )
throws IOException
{
NativeSchemaIndexHeaderReader headerReader = new NativeSchemaIndexHeaderReader();
GBPTree.readHeader( pageCache, indexFile, layout, headerReader );
GBPTree.readHeader( pageCache, indexFile, headerReader );
return headerReader.failureMessage;
}
}
6 changes: 3 additions & 3 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialCRSSchemaIndex.java
View file
Expand Up @@ -44,8 +44,8 @@
import org.neo4j.io.pagecache.PageCache;
import org.neo4j.kernel.api.index.IndexDirectoryStructure;
import org.neo4j.kernel.api.index.IndexEntryUpdate;
import org.neo4j.kernel.api.index.IndexUpdater;
import org.neo4j.kernel.api.index.IndexProvider;
import org.neo4j.kernel.api.index.IndexUpdater;
import org.neo4j.kernel.api.schema.index.SchemaIndexDescriptor;
import org.neo4j.kernel.impl.api.index.sampling.IndexSamplingConfig;
import org.neo4j.kernel.impl.index.schema.NativeSchemaIndexPopulator.IndexUpdateApply;
Expand Down Expand Up @@ -327,12 +327,12 @@ public boolean indexExists()

public String readPopulationFailure() throws IOException
{
return NativeSchemaIndexes.readFailureMessage( pageCache, indexFile, layout );
return NativeSchemaIndexes.readFailureMessage( pageCache, indexFile );
}

public InternalIndexState readState() throws IOException
{
return NativeSchemaIndexes.readState( pageCache, indexFile, layout );
return NativeSchemaIndexes.readState( pageCache, indexFile );
}

private synchronized void create() throws IOException
Expand Down
4 changes: 2 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/TemporalIndexProvider.java
View file
Expand Up @@ -84,7 +84,7 @@ public String getPopulationFailure( long indexId, SchemaIndexDescriptor descript
{
for ( TemporalIndexFiles.FileLayout subIndex : temporalIndexFiles.existing() )
{
String indexFailure = NativeSchemaIndexes.readFailureMessage( pageCache, subIndex.indexFile, subIndex.layout );
String indexFailure = NativeSchemaIndexes.readFailureMessage( pageCache, subIndex.indexFile );
if ( indexFailure != null )
{
return indexFailure;
Expand All @@ -108,7 +108,7 @@ public InternalIndexState getInitialState( long indexId, SchemaIndexDescriptor d
{
try
{
switch ( NativeSchemaIndexes.readState( pageCache, subIndex.indexFile, subIndex.layout ) )
switch ( NativeSchemaIndexes.readState( pageCache, subIndex.indexFile ) )
{
case FAILED:
return InternalIndexState.FAILED;
Expand Down

0 comments on commit a46adca

Please sign in to comment.