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

[FD-149680] Retain triggers by default fix #14

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bhuvanesh-sankar
Copy link
Collaborator

@bhuvanesh-sankar bhuvanesh-sankar commented Nov 4, 2024

Currently as per the fix in https://github.com/freshworks/large-hadron-migrator/pull/11 we retain triggers only if retain_triggers flag is set to true in migration script. This leads to issues when this flag is missed to be passed while running migration.
Due to this we have to update the logic to retain triggers by default, so we have to remove the retain_triggers check in the logic and preserve triggers by default.

Fix made:
Removed the retain_triggers flag check in the preserve trigger logic

@bhuvanesh-sankar bhuvanesh-sankar changed the title Retain triggers by default fix [FD-149680] Retain triggers by default fix Nov 4, 2024
@@ -23,12 +23,11 @@ class LockedSwitcher

attr_reader :connection

def initialize(migration, connection = nil, retain_triggers = false)

Choose a reason for hiding this comment

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

@bhuvanesh-sankar can we not instead make this as retain_triggers = true?
That is by default if the flag is not passed, it will be true and you would also have the option to pass it as false - retaining the original percona behavior. What do you think?

I don't see a use case though to not retain the triggers.

@ImAjayS Your thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thought of this initially, but as you mentioned we don't have an use case to not retain the triggers. So it would be an added logic which is of less / no use in the system. That's why didn't consider this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants