Skip to content

Commit

Permalink
Includes stack trace of unexpected checker dispatch error
Browse files Browse the repository at this point in the history
Previously only the exception message was logged into the consistency report.
This in addition to the exception being swallowed created second failures that
were hard to find root cause of.
  • Loading branch information
tinwelint committed Jan 23, 2017
1 parent 50810bc commit 000d08b
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 4 deletions.
Expand Up @@ -64,10 +64,19 @@ public void process( NodeRecord nodeRecord )
try ( MandatoryProperties.Check<NodeRecord,ConsistencyReport.NodeConsistencyReport> mandatoryCheck =
mandatoryProperties.apply( nodeRecord ) )
{
for ( PropertyRecord property : client.getPropertiesFromCache() )
Iterable<PropertyRecord> properties = client.getPropertiesFromCache();

// We do this null-check here because even if nodeIndexCheck should provide the properties for us,
// or an empty list at least, it may fail in one way or another and exception be caught by
// broad exception handler in reporter. The caught exception will produce an ERROR so it will not
// go by unnoticed.
if ( properties != null )
{
reporter.forProperty( property, propertyCheck );
mandatoryCheck.receive( ChainCheck.keys( property ) );
for ( PropertyRecord property : properties )
{
reporter.forProperty( property, propertyCheck );
mandatoryCheck.receive( ChainCheck.keys( property ) );
}
}
}
}
Expand Down
Expand Up @@ -53,6 +53,7 @@
import static java.util.Arrays.asList;

import static org.neo4j.helpers.Exceptions.launderedException;
import static org.neo4j.helpers.Exceptions.stringify;
import static org.neo4j.helpers.Exceptions.withCause;

public class ConsistencyReporter implements ConsistencyReport.Reporter
Expand Down Expand Up @@ -126,7 +127,10 @@ void dispatch( RecordType type, ProxyFactory<REPORT> factory, RECORD record, Rec
}
catch ( Exception e )
{
handler.report.error( type, record, "Failed to check record: " + e.getMessage(), new Object[0] );
// This is a rare event and exposing the stack trace is a good idea, otherwise we
// can only see that something went wrong, not at all what.
handler.report.error( type, record, "Failed to check record: " + stringify( e ),
new Object[0] );
}
handler.updateSummary();
}
Expand Down
Expand Up @@ -24,6 +24,7 @@
import org.hamcrest.TypeSafeMatcher;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TestName;
import org.junit.rules.TestRule;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
Expand All @@ -38,11 +39,13 @@
import java.lang.reflect.Type;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicReference;

import org.neo4j.consistency.RecordType;
import org.neo4j.consistency.checking.CheckerEngine;
import org.neo4j.consistency.checking.ComparativeRecordChecker;
import org.neo4j.consistency.checking.RecordCheck;
import org.neo4j.consistency.report.ConsistencyReport.NodeConsistencyReport;
import org.neo4j.consistency.store.RecordAccess;
import org.neo4j.consistency.store.RecordReference;
import org.neo4j.consistency.store.synthetic.CountsEntry;
Expand All @@ -64,9 +67,14 @@
import org.neo4j.kernel.impl.store.record.RelationshipTypeTokenRecord;
import org.neo4j.kernel.impl.store.record.SchemaRule;

import static org.hamcrest.Matchers.containsString;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.argThat;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
Expand All @@ -84,6 +92,9 @@ public class ConsistencyReporterTest
{
public static class TestReportLifecycle
{
@Rule
public final TestName testName = new TestName();

@Test
public void shouldSummarizeStatisticsAfterCheck()
{
Expand Down Expand Up @@ -139,6 +150,57 @@ public void shouldOnlySummarizeStatisticsWhenAllReferencesAreChecked()
verify( summary ).update( RecordType.PROPERTY, 0, 0 );
verifyNoMoreInteractions( summary );
}

@Test
public void shouldIncludeStackTraceInUnexpectedCheckException() throws Exception
{
// GIVEN
ConsistencySummaryStatistics summary = mock( ConsistencySummaryStatistics.class );
RecordAccess records = mock( RecordAccess.class );
final AtomicReference<String> loggedError = new AtomicReference<>();
InconsistencyLogger logger = new InconsistencyLogger()
{
@Override
public void error( RecordType recordType, AbstractBaseRecord record, String message, Object[] args )
{
assertTrue( loggedError.compareAndSet( null, message ) );
}

@Override
public void error( RecordType recordType, AbstractBaseRecord oldRecord, AbstractBaseRecord newRecord,
String message, Object[] args )
{
assertTrue( loggedError.compareAndSet( null, message ) );
}

@Override
public void warning( RecordType recordType, AbstractBaseRecord record, String message, Object[] args )
{
}

@Override
public void warning( RecordType recordType, AbstractBaseRecord oldRecord, AbstractBaseRecord newRecord,
String message, Object[] args )
{
}
};
InconsistencyReport inconsistencyReport = new InconsistencyReport( logger, summary );
ConsistencyReporter reporter = new ConsistencyReporter( records, inconsistencyReport );
NodeRecord node = new NodeRecord( 10 );
RecordCheck<NodeRecord,NodeConsistencyReport> checker = mock( RecordCheck.class );
RuntimeException exception = new RuntimeException( "My specific exception" );
doThrow( exception ).when( checker )
.check( any( NodeRecord.class ), any( CheckerEngine.class ), any( RecordAccess.class ) );

// WHEN
reporter.forNode( node, checker );

// THEN
assertNotNull( loggedError.get() );
String error = loggedError.get();
assertThat( error, containsString( "at " ) );
assertThat( error, containsString( testName.getMethodName() ) );
}
}

@RunWith(Parameterized.class)
Expand Down

0 comments on commit 000d08b

Please sign in to comment.