Skip to content

Commit

Permalink
Fix reading of props with different block sizes
Browse files Browse the repository at this point in the history 
Currently we have two dynamic stores for long property values -
DynamicStringStore and DynamicArrayStore. They can have their block sizes
configured separately via string_block_size and array_block_size internal
properties. StorePropertyCursor and StorePropertyPayloadCursor are used to
read actual property values. Payload cursor wraps a dynamic record cursor tries
to reuse most of it's internal state.

Issue occurred because StorePropertyPayloadCursor tried to use same dynamic
record cursor for reading from both dynamic string and array store, which can
have different block size.

This commit makes property payload cursor use different cursors for dynamic
string store and dynamic array store. It also restructures
StorePropertyPayloadCursorTest because previously some basic tests were not
executed as they were not inside an inner class.

Fixes #6133
  • Loading branch information
@lutovich
lutovich committed Dec 18, 2015
1 parent 4380013 commit f958be0
Show file tree
Hide file tree
Showing 3 changed files with 175 additions and 94 deletions.
32 changes: 19 additions & 13 deletions ...nity/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java
View file
Expand Up @@ -81,7 +81,8 @@ class StorePropertyPayloadCursor
private final DynamicStringStore stringStore;
private final DynamicArrayStore arrayStore;

private AbstractDynamicStore.DynamicRecordCursor recordCursor;
private AbstractDynamicStore.DynamicRecordCursor stringRecordCursor;
private AbstractDynamicStore.DynamicRecordCursor arrayRecordCursor;
private ByteBuffer buffer = cachedBuffer;

private final long[] data = new long[MAX_NUMBER_OF_PAYLOAD_LONG_ARRAY];
Expand Down Expand Up @@ -202,11 +203,15 @@ String stringValue()
assertOfType( STRING );
try
{
readFromStore( stringStore );
if ( stringRecordCursor == null )
{
stringRecordCursor = stringStore.newDynamicRecordCursor();
}
readFromStore( stringStore, stringRecordCursor );
}
catch ( IOException e )
{
throw new UnderlyingStorageException( e );
throw new UnderlyingStorageException( "Unable to read string value", e );
}
buffer.flip();
return UTF8.decode( buffer.array(), 0, buffer.limit() );
Expand All @@ -224,11 +229,15 @@ Object arrayValue()
assertOfType( ARRAY );
try
{
readFromStore( arrayStore );
if ( arrayRecordCursor == null )
{
arrayRecordCursor = arrayStore.newDynamicRecordCursor();
}
readFromStore( arrayStore, arrayRecordCursor );
}
catch ( IOException e )
{
throw new UnderlyingStorageException( e );
throw new UnderlyingStorageException( "Unable to read array value", e );
}
buffer.flip();
return readArrayFromBuffer( buffer );
Expand All @@ -247,23 +256,20 @@ private int currentBlocksUsed()
private Bits valueAsBits()
{
Bits bits = Bits.bits( MAX_BYTES_IN_SHORT_STRING_OR_SHORT_ARRAY );
for ( int i = 0; i < currentBlocksUsed(); i++ )
int blocksUsed = currentBlocksUsed();
for ( int i = 0; i < blocksUsed; i++ )
{
bits.put( data[position + i] );
}
return bits;
}

private void readFromStore( AbstractDynamicStore store ) throws IOException
private void readFromStore( AbstractDynamicStore store, AbstractDynamicStore.DynamicRecordCursor cursor )
throws IOException
{
if ( recordCursor == null )
{
recordCursor = store.newDynamicRecordCursor();
}

buffer.clear();
long startBlockId = PropertyBlock.fetchLong( currentHeader() );
try ( GenericCursor<DynamicRecord> records = store.getRecordsCursor( startBlockId, true, recordCursor ) )
try ( GenericCursor<DynamicRecord> records = store.getRecordsCursor( startBlockId, true, cursor ) )
{
while ( records.next() )
{
Expand Down
165 changes: 112 additions & 53 deletions .../kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursorTest.java
View file
Expand Up @@ -19,6 +19,7 @@
*/
package org.neo4j.kernel.impl.api.store;

import org.apache.commons.lang3.RandomStringUtils;
import org.junit.Test;
import org.junit.experimental.runners.Enclosed;
import org.junit.runner.RunWith;
Expand All @@ -34,6 +35,7 @@
import org.neo4j.helpers.collection.IteratorUtil;
import org.neo4j.io.pagecache.PageCursor;
import org.neo4j.io.pagecache.StubPageCursor;
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;
Expand All @@ -43,8 +45,15 @@
import org.neo4j.kernel.impl.store.record.PropertyBlock;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyBoolean;
import static org.mockito.Matchers.anyLong;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.neo4j.kernel.impl.api.store.StorePropertyCursor.payloadValueAsObject;
import static org.neo4j.kernel.impl.api.store.StorePropertyPayloadCursorTest.Param.param;
import static org.neo4j.kernel.impl.api.store.StorePropertyPayloadCursorTest.Param.paramArg;
Expand All @@ -54,76 +63,104 @@
@RunWith( Enclosed.class )
public class StorePropertyPayloadCursorTest
{
@Test
public void shouldBeOkToClearUnusedCursor()
public static class BasicContract
{
// Given
StorePropertyPayloadCursor cursor = newCursor( "cat-dog" );
@Test
public void shouldBeOkToClearUnusedCursor()
{
// Given
StorePropertyPayloadCursor cursor = newCursor( "cat-dog" );

// When
cursor.clear();
// When
cursor.clear();

// Then
// clear() on an unused cursor works just fine
}
// Then
// clear() on an unused cursor works just fine
}

@Test
public void shouldBeOkToClearPartiallyExhaustedCursor()
{
// Given
StorePropertyPayloadCursor cursor = newCursor( 1, 2, 3L );
@Test
public void shouldBeOkToClearPartiallyExhaustedCursor()
{
// Given
StorePropertyPayloadCursor cursor = newCursor( 1, 2, 3L );

cursor.next();
cursor.next();
cursor.next();
cursor.next();

// When
cursor.clear();
// When
cursor.clear();

// Then
// clear() on an used cursor works just fine
}
// Then
// clear() on an used cursor works just fine
}

@Test
public void shouldBeOkToClearExhaustedCursor()
{
// Given
StorePropertyPayloadCursor cursor = newCursor( 1, 2, 3 );
@Test
public void shouldBeOkToClearExhaustedCursor()
{
// Given
StorePropertyPayloadCursor cursor = newCursor( 1, 2, 3 );

cursor.next();
cursor.next();
cursor.next();
cursor.next();
cursor.next();
cursor.next();

// When
cursor.clear();
// When
cursor.clear();

// Then
// clear() on an exhausted cursor works just fine
}
// Then
// clear() on an exhausted cursor works just fine
}

@Test
public void shouldBePossibleToCallClearOnEmptyCursor()
{
// Given
StorePropertyPayloadCursor cursor = newCursor();
@Test
public void shouldBePossibleToCallClearOnEmptyCursor()
{
// Given
StorePropertyPayloadCursor cursor = newCursor();

// When
cursor.clear();

// When
cursor.clear();
// Then
// clear() on an empty cursor works just fine
}

// Then
// clear() on an empty cursor works just fine
}
@Test
public void shouldBePossibleToCallNextOnEmptyCursor()
{
// Given
StorePropertyPayloadCursor cursor = newCursor();

@Test
public void shouldBePossibleToCallNextOnEmptyCursor()
{
// Given
StorePropertyPayloadCursor cursor = newCursor();
// When
cursor.next();

// Then
// next() on an empty cursor works just fine
}

@Test
public void shouldUseDynamicStringAndArrayStoresThroughDifferentCursors()
{
// Given
DynamicStringStore dynamicStringStore = newDynamicStoreMock( DynamicStringStore.class );
DynamicArrayStore dynamicArrayStore = newDynamicStoreMock( DynamicArrayStore.class );

// When
cursor.next();
String string = RandomStringUtils.randomAlphanumeric( 5000 );
byte[] array = RandomStringUtils.randomAlphanumeric( 10000 ).getBytes();
StorePropertyPayloadCursor cursor = newCursor( dynamicStringStore, dynamicArrayStore, string, array );

// When
assertTrue( cursor.next() );
assertNotNull( cursor.stringValue() );

// Then
// next() on an empty cursor works just fine
assertTrue( cursor.next() );
assertNotNull( cursor.arrayValue() );

assertFalse( cursor.next() );

// Then
verify( dynamicStringStore ).newDynamicRecordCursor();
verify( dynamicArrayStore ).newDynamicRecordCursor();
}
}

@RunWith( Parameterized.class )
Expand Down Expand Up @@ -390,6 +427,12 @@ private static StorePropertyPayloadCursor newCursor( Object... values )
DynamicStringStore dynamicStringStore = mock( DynamicStringStore.class );
DynamicArrayStore dynamicArrayStore = mock( DynamicArrayStore.class );

return newCursor( dynamicStringStore, dynamicArrayStore, values );
}

private static StorePropertyPayloadCursor newCursor( DynamicStringStore dynamicStringStore,
DynamicArrayStore dynamicArrayStore, Object... values )
{
StorePropertyPayloadCursor cursor = new StorePropertyPayloadCursor( dynamicStringStore, dynamicArrayStore );

PageCursor pageCursor = newPageCursor( values );
Expand Down Expand Up @@ -424,6 +467,22 @@ private static PageCursor newPageCursor( Object... values )
return new StubPageCursor( 1, page );
}

private static <S extends AbstractDynamicStore> S newDynamicStoreMock( Class<S> clazz )
{
AbstractDynamicStore.DynamicRecordCursor recordCursor = mock( AbstractDynamicStore.DynamicRecordCursor.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 );

S store = mock( clazz );
when( store.newDynamicRecordCursor() ).thenReturn( mock( AbstractDynamicStore.DynamicRecordCursor.class ) );
when( store.getRecordsCursor( anyLong(), anyBoolean(), any( AbstractDynamicStore.DynamicRecordCursor.class ) ) )
.thenReturn( recordCursor );

return store;
}

static class Param
{
final Object value;
Expand Down
72 changes: 44 additions & 28 deletions community/kernel/src/test/java/org/neo4j/kernel/impl/core/LargePropertiesIT.java
View file
Expand Up @@ -19,44 +19,60 @@
*/
package org.neo4j.kernel.impl.core;

import org.junit.After;
import org.junit.Before;
import org.junit.ClassRule;
import org.apache.commons.lang3.RandomStringUtils;
import org.junit.Rule;
import org.junit.Test;

import org.neo4j.graphdb.GraphDatabaseService;
import org.neo4j.graphdb.Node;
import org.neo4j.test.DatabaseRule;
import org.neo4j.test.GraphTransactionRule;
import org.neo4j.test.ImpermanentDatabaseRule;
import org.neo4j.graphdb.Transaction;
import org.neo4j.graphdb.factory.GraphDatabaseSettings;
import org.neo4j.test.EphemeralFileSystemRule;
import org.neo4j.test.TestGraphDatabaseFactory;

import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;

public class LargePropertiesIT
{
@ClassRule
public static DatabaseRule db = new ImpermanentDatabaseRule();

@Rule
public GraphTransactionRule tx = new GraphTransactionRule( db );

private Node node;

@Before
public void createInitialNode()
{
node = db.getGraphDatabaseService().createNode();
}
public final EphemeralFileSystemRule fs = new EphemeralFileSystemRule();

@After
public void deleteInitialNode()
{
node.delete();
}

@Test
public void testLargeProperties()
public void readArrayAndStringPropertiesWithDifferentBlockSizes()
{
byte[] bytes = new byte[10*1024*1024];
node.setProperty( "large_array", bytes );
node.setProperty( "large_string", new String( bytes ) );
String stringValue = RandomStringUtils.randomAlphanumeric( 10000 );
byte[] arrayValue = RandomStringUtils.randomAlphanumeric( 10000 ).getBytes();

GraphDatabaseService db = new TestGraphDatabaseFactory()
.setFileSystem( fs.get() )
.newImpermanentDatabaseBuilder()
.setConfig( GraphDatabaseSettings.string_block_size, "1024" )
.setConfig( GraphDatabaseSettings.array_block_size, "2048" )
.newGraphDatabase();
try
{
long nodeId;
try ( Transaction tx = db.beginTx() )
{
Node node = db.createNode();
nodeId = node.getId();
node.setProperty( "string", stringValue );
node.setProperty( "array", arrayValue );
tx.success();
}

try ( Transaction tx = db.beginTx() )
{
Node node = db.getNodeById( nodeId );
assertEquals( stringValue, node.getProperty( "string" ) );
assertArrayEquals( arrayValue, (byte[]) node.getProperty( "array" ) );
tx.success();
}
}
finally
{
db.shutdown();
}
}
}

0 comments on commit f958be0

Please sign in to comment.