Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[jMd3ZuqI] Fix arrow flaky tests #519

Merged
merged 4 commits into from
Nov 6, 2023
Merged

[jMd3ZuqI] Fix arrow flaky tests #519

merged 4 commits into from
Nov 6, 2023

Conversation

ncordon
Copy link
Collaborator

@ncordon ncordon commented Nov 1, 2023

What

Uses just one thread in the arrow export procedures to fix flaky tests:

java.lang.ArrayIndexOutOfBoundsException: Index 0 out of bounds for length 0
    at org.eclipse.collections.impl.set.strategy.mutable.UnifiedSetWithHashingStrategy.copyBucketToArray(UnifiedSetWithHashingStrategy.java:1732)
    at org.eclipse.collections.impl.set.strategy.mutable.UnifiedSetWithHashingStrategy.copyToArray(UnifiedSetWithHashingStrategy.java:1718)
    at org.eclipse.collections.impl.set.strategy.mutable.UnifiedSetWithHashingStrategy.toArray(UnifiedSetWithHashingStrategy.java:1767)
    at java.base/java.util.Collection.toArray(Collection.java:413)
    at org.neo4j.kernel.impl.api.CloseableResourceManager.closeAllCloseableResources(CloseableResourceManager.java:62)
    at org.neo4j.kernel.impl.api.KernelStatement.cleanupResources(KernelStatement.java:247)
    at org.neo4j.kernel.impl.api.KernelStatement.close(KernelStatement.java:128)
    at org.neo4j.kernel.impl.query.Neo4jTransactionalContext.closeStatement(Neo4jTransactionalContext.java:161)
    at org.neo4j.kernel.impl.query.Neo4jTransactionalContext.close(Neo4jTransactionalContext.java:150)
    at org.neo4j.cypher.internal.runtime.interpreted.SingleThreadedTransactionalContextWrapper.close(TransactionalContextWrapper.scala:135)
    at org.neo4j.cypher.internal.runtime.interpreted.DelegatingQueryTransactionalContext.close(DelegatingQueryContext.scala:747)
    at org.neo4j.cypher.internal.planning.ExceptionTranslatingReadQueryContext$ExceptionTranslatingTransactionalContext$.close(ExceptionTranslatingQueryContext.scala:546)
    at org.neo4j.cypher.internal.CypherCurrentCompiler$CypherExecutableQuery.$anonfun$execute$2(CypherCurrentCompiler.scala:393)
    at org.neo4j.cypher.internal.result.StandardInternalExec...

Why

Because if we have two threads, if one of the threads frees the result stream that the outer one owns (this happens when there are not any more elements in the stream), the outer transaction thread might not see that change (https://stackoverflow.com/a/2787581). This could be the cause why we are experiencing some flakiness.

It makes sense to change the code anyway because it is really convoluted and there's no reason why we need a new transaction in another thread.

We should review the use of that QueueSplitIterator everywhere.

@ncordon ncordon added bug Something isn't working dev labels Nov 1, 2023
@ncordon ncordon changed the title Fix arrow flaky tests [jMd3ZuqI] Fix arrow flaky tests Nov 1, 2023
Copy link
Contributor

@gem-neo4j gem-neo4j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a nice cleanup to see :D Can you make a card for the cleanup of QueueSplitIterator in general?

@ncordon
Copy link
Collaborator Author

ncordon commented Nov 6, 2023

/spotlessApply

@ncordon ncordon merged commit 50adcfe into dev Nov 6, 2023
14 checks passed
@ncordon ncordon deleted the fix-arrow-threads branch November 6, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants