Don’t swallow the cause of a TX termination. #731
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are a couple of scenarions in which a transaction gets terminated. The cause will be propagated by the pull handler and the transaction will be marked accordingly. There might be a chance that a degration from a leader to a follower on the server side happens in between to calls to a run method on that transaction: The transaction is still open, but cannot run queries any longer. Outside a transactional function this leads correctly to a ClientException. Inside a transactional function, this must not happen. The retry logic must be able to find the cause of the termination and if there’s any, it should judge on the cause if it retries or not. This PR changes the following: - Add a StateHolder to the UnmangedTransaction - The holder is necessary to keep the single field volatie - The holder holds the state and a possible cause of termination - The holder is able to determine whether a session is still open or not. - It removes markTerminated from InternalAsyncTransaction as it was used only for tests.
This removes the circular dependency between holder and state. I had introduced it cause I wanted to avoid object allocation. This solution here is better: It also creates only 3 different holders for non terminated states, but without the circular dependency.
gjmwoods
approved these changes
Jun 8, 2020
gjmwoods
added a commit
to gjmwoods/neo4j-java-driver
that referenced
this pull request
Jun 10, 2020
* Don’t swallow the cause of a TX termination. There are a couple of scenarions in which a transaction gets terminated. The cause will be propagated by the pull handler and the transaction will be marked accordingly. There might be a chance that a degration from a leader to a follower on the server side happens in between to calls to a run method on that transaction: The transaction is still open, but cannot run queries any longer. Outside a transactional function this leads correctly to a ClientException. Inside a transactional function, this must not happen. The retry logic must be able to find the cause of the termination and if there’s any, it should judge on the cause if it retries or not. This PR changes the following: - Add a StateHolder to the UnmangedTransaction - The holder is necessary to keep the single field volatie - The holder holds the state and a possible cause of termination - The holder is able to determine whether a session is still open or not. - It removes markTerminated from InternalAsyncTransaction as it was used only for tests. * Polishing. This removes the circular dependency between holder and state. I had introduced it cause I wanted to avoid object allocation. This solution here is better: It also creates only 3 different holders for non terminated states, but without the circular dependency. Co-authored-by: Gregory Woods <[email protected]>
gjmwoods
added a commit
that referenced
this pull request
Jun 10, 2020
* Don’t swallow the cause of a TX termination. There are a couple of scenarions in which a transaction gets terminated. The cause will be propagated by the pull handler and the transaction will be marked accordingly. There might be a chance that a degration from a leader to a follower on the server side happens in between to calls to a run method on that transaction: The transaction is still open, but cannot run queries any longer. Outside a transactional function this leads correctly to a ClientException. Inside a transactional function, this must not happen. The retry logic must be able to find the cause of the termination and if there’s any, it should judge on the cause if it retries or not. This PR changes the following: - Add a StateHolder to the UnmangedTransaction - The holder is necessary to keep the single field volatie - The holder holds the state and a possible cause of termination - The holder is able to determine whether a session is still open or not. - It removes markTerminated from InternalAsyncTransaction as it was used only for tests. * Polishing. This removes the circular dependency between holder and state. I had introduced it cause I wanted to avoid object allocation. This solution here is better: It also creates only 3 different holders for non terminated states, but without the circular dependency. Co-authored-by: Gregory Woods <[email protected]> Co-authored-by: Michael Simons <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are a couple of scenarions in which a transaction gets terminated. The cause will be propagated by the pull handler and the transaction will be marked accordingly. There might be a chance that a degration from a leader to a follower on the server side happens in between to calls to a run method on that transaction: The transaction is still open, but cannot run queries any longer. Outside a transactional function this leads correctly to a ClientException. Inside a transactional function, this must not happen.
The retry logic must be able to find the cause of the termination and if there’s any, it should judge on the cause if it retries or not.
This PR changes the following: