-
Notifications
You must be signed in to change notification settings - Fork 30
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
[H5vz3qhy] Prevent cascade trigger transactions #346
Conversation
@@ -35,6 +35,7 @@ | |||
public class TriggerHandler extends LifecycleAdapter implements TransactionEventListener<Void> { | |||
|
|||
private enum Phase {before, after, rollback, afterAsync} | |||
private static final Map<String, Object> TRIGGER_META = Map.of("trigger", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Map.of("apoc.trigger", true)
would be better in my opinion. Just in case people start seeing things in their transactions they don't know where they came from, that's more self explanatory than just trigger
// if `txData.metaData()` is equal to TRIGGER_META, it means that the transaction | ||
// if it is equal, it means that the transaction comes from another TriggerHandler transaction, | ||
// therefore the execution must be blocked to prevent a deadlock due to cascading transactions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// if `txData.metaData()` is equal to TRIGGER_META, it means that the transaction | |
// if it is equal, it means that the transaction comes from another TriggerHandler transaction, | |
// therefore the execution must be blocked to prevent a deadlock due to cascading transactions | |
// if `txData.metaData()` is equal to TRIGGER_META, it means that the transaction | |
// comes from another TriggerHandler transaction, | |
// therefore the execution must be blocked to prevent a deadlock due to cascading transactions |
// if it is equal, it means that the transaction comes from another TriggerHandler transaction, | ||
// therefore the execution must be blocked to prevent a deadlock due to cascading transactions | ||
Map<String, Object> metaData = txData.metaData(); | ||
if (metaData.equals(TRIGGER_META)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is personal preference but if you extract this block outside to a function:
public Boolean isTransactionCreatedByTrigger(TransactionData txData) {
Map<String, Object> metaData = txData.metaData();
return metaData.equals(TRIGGER_META);
}
then this method becomes more self-documented
* [H5vz3qhy] Prevent cascade trigger transactions * [H5vz3qhy] Changed flaky test * [H5vz3qhy] Small changes review
* [H5vz3qhy] Prevent cascade trigger transactions * [H5vz3qhy] Changed flaky test * [H5vz3qhy] Small changes review
* [H5vz3qhy] Prevent cascade trigger transactions * [H5vz3qhy] Changed flaky test * [H5vz3qhy] Small changes review
Added a
CALL tx.setMetaData(<TRIGGER_META>)
procedure in transactions with phaseafter
andafterAsync
,in order to prevent cascade trigger transactions coming from
TriggerHandler
itself,so that we can avoid some deadlock conditions.
I don't know if it is somehow directly related to this pr,
but I had to change the
testRemoveNode
too,because it sometimes failed as the previous
CREATE (f:Foo)
had id = 0.