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

[HTtgtRFX] Fix bug in triggers #547

Merged
merged 3 commits into from
Dec 4, 2023
Merged

[HTtgtRFX] Fix bug in triggers #547

merged 3 commits into from
Dec 4, 2023

Conversation

loveleif
Copy link
Contributor

@loveleif loveleif commented Nov 29, 2023

  • Fix bug in triggers, last update was too early.
  • Fix flaky test assertion in TriggerEnterpriseFeaturesTest.stressTest. I can't find anything wrong with the assertions, but they sometimes caused EntityNotFoundException during installing of triggers and hopefully this helps.
  • Apply code style to the whole project (this was accidental, but I guess I might as well keep it).

@loveleif loveleif added the dev label Nov 29, 2023
@loveleif loveleif changed the title [HTtgtRFX] Fix flaky stress test [HTtgtRFX] Fix bug in triggers Nov 29, 2023
}
}

record DropRandomTriggers(Driver driver, String db, int iterations) implements Runnable {
Copy link
Contributor Author

@loveleif loveleif Nov 30, 2023

Choose a reason for hiding this comment

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

Note, before we dropped triggers at random concurrently to creating them. I see no reason why that should not be possible. But since install trigger failed sometimes (EntityNotFound) I change it here to not drop and install at the same time. The important thing is that existing triggers run while we add and remove others, and that is still covered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I am just a bit slow this afternoon, but I'm a little confused as to how this is working, so in order it creates the Triggers, but deletes them randomly, so it may delete a trigger that isn't yet created? and won't delete all of them I guess?

I guess that is fine, just trying to think through the logic :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not the clearest test, but it was really good at reproducing a previous bug. Your statements are correct. The randomness does not serve that much purpose now. I guess I kept it to have some triggers live on for different amounts of time in case that makes a difference.

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 looks good to me :) Slightly confused by the test, but it wasn't your changes that were confusing at least

}
}

record DropRandomTriggers(Driver driver, String db, int iterations) implements Runnable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I am just a bit slow this afternoon, but I'm a little confused as to how this is working, so in order it creates the Triggers, but deletes them randomly, so it may delete a trigger that isn't yet created? and won't delete all of them I guess?

I guess that is fine, just trying to think through the logic :)

@loveleif loveleif merged commit e8ac28f into dev Dec 4, 2023
23 checks passed
@loveleif loveleif deleted the fix-flaky-stress-test branch December 4, 2023 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants