Procedures now close acquired KernelStatements #9764
Conversation
Verified in test by performing two index reads with intermediate create. Second read should see result of create but will not if KernelStatement has leaked, because Lucene reader from first match will be reused for second read.
|
Merge commit to 3.2 here https://github.com/burqen/neo4j/tree/3.2-procedures-close-kernel-statement-merge-from-3.1 |
| Iterator<RelationshipType> relationshipTypeIterator = | ||
| graphDatabaseAPI.getAllRelationshipTypesInUse().iterator(); | ||
| while ( relationshipTypeIterator.hasNext() ) | ||
| try ( Transaction transaction = graphDatabaseAPI.beginTx(); ) |
There was a problem hiding this comment.
Do we need this inner transaction? We are already in a transaction, so this will be a dummy.
There was a problem hiding this comment.
I would guess not, but cleaning this up is outside the scope of this PR.
There was a problem hiding this comment.
Looks good, a couple of small comments. While this change cleaned up a lot of incorrect statements usages here and there I think we need more strict eyes on those. We need the tracker that will track open statements in the transaction and will fail it in the case when it will be closed without statements being closed across all our tests at least, sounds like a good idea?
| RelationshipType relationshipType = relationshipTypeIterator.next(); | ||
| String relationshipTypeGetName = relationshipType.name(); | ||
| int relId = readOperations.relationshipTypeGetForName( relationshipTypeGetName ); | ||
| ResourceIterator<Label> labelsInUse = graphDatabaseAPI.getAllLabelsInUse().iterator(); |
There was a problem hiding this comment.
this resource iterator should be closed as well
| } | ||
| try ( Transaction tx = db.beginTx() ) | ||
| { | ||
| db.schema().awaitIndexesOnline( 1, TimeUnit.SECONDS ); |
There was a problem hiding this comment.
this can be not enough on latest 3.3 version
There was a problem hiding this comment.
Hmm... set to 5 just in case then.
|
|
||
| private String buildProcedureQuery( String procedureName, List<Object> parameters ) | ||
| { | ||
| StringBuilder sb = new StringBuilder(); |
There was a problem hiding this comment.
String joiner can be used here to simplify looping and checks, not that important in the same time
There was a problem hiding this comment.
that is what i thinking about:
StringJoiner stringJoiner = new StringJoiner( ",", "CALL " + procedureName + "(", ")" );
for ( Object parameter : parameters )
{
stringJoiner.add( parameter.toString() );
}
return stringJoiner.toString();
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| public class ProcedureResourcesIT |
There was a problem hiding this comment.
different versions will contain different procedures so this test will need some adaptations during merges be careful there 👀
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| public class ProcedureResourcesIT |
There was a problem hiding this comment.
its' also strange that this test does not catch a case where we still have one statement that is not closed because of not closed resource iterable
There was a problem hiding this comment.
Problem was there was no relationships in the dataset which meant that code path was never hit. Fixed now.
|
For completeness. Here is an attempt at tracking open statements: #9794 |
|
@MishaDemianenko Comments addressed. Will take care to adapt test during forward merge! |
| } | ||
| sb.append( joiner.toString() ); | ||
| sb.append( ")" ); | ||
| System.out.println( sb.toString() ); |
There was a problem hiding this comment.
in any case you do not need to print this string
There was a problem hiding this comment.
@burqen 2 small comments otherwise looks good will approve now but please take a look on them
|
@MishaDemianenko Ping 👍 |
|
@burqen Pong, whats up?) |
|
👍 |
Verified in test by performing two index reads with
intermediate create. Second read should see result
of create but will not if KernelStatement has leaked,
because Lucene reader from first match will be reused
for second read.