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

Add --test-on-replica-manual-replication-control flag #174

Merged

Conversation

pbitty
Copy link
Contributor

@pbitty pbitty commented Aug 18, 2016

This will wait indefinitely for the replication status to change.
This allows us to run test schema changes in RDS without needing
custom RDS commands in gh-ost.

Closes #162

This will wait indefinitely for the replication status to change.
This allows us to run test schema changes in RDS without needing
custom RDS commands in gh-ost.
}
migrationContext.TestOnReplica = true
migrationContext.ManualReplicationControl = true
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shlomi-noach , in my opinion the code reads more complicated to me when there are two flags that have the same core functionality, with this one slight variation.

I think the semantics would be more clear if we still used the --test-on-replica flag and had a separate flag that just customized this one aspect of its behavior. So if one wants to have manual replication control, one would use the args:

--test-on-replica --manual-replication-control

Without the extra flag, --test-on-replica would behave as it always has.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

That was my original intention. The flag is good to be named --test-on-replica-skip-stop-replica or whatever, but it would be an addon to --test-on-replica. Not only are the flags not mutually exclusive, the --test-on-replica-skip-stop-replica is only applicable when --test-on-replica exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I understand now. My apologies for the confusion. I'll update the logic.

@shlomi-noach
Copy link
Contributor

See comments inline

@pbitty
Copy link
Contributor Author

pbitty commented Aug 19, 2016

I've updated the flag and behavior we discussed. It is now an add-on flag to --test-on-replica and has some warnings about the impact of using it without another means of stopping replication.

if !migrationContext.TestOnReplica {
log.Fatalf("--test-on-replica-skip-replica-stop requires --test-on-replica to be enabled")
}
log.Warning("--test-on-replica-skip-replica-stop enabled. We will not stop replication before cut-over. Ensure you have a plugin that does this.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made mention of plugins (prematurely) in the warning because I don't think this feature will really work without them.

@pbitty
Copy link
Contributor Author

pbitty commented Aug 23, 2016

Not at all. I'll take a look first thing tomorrow.

@pbitty
Copy link
Contributor Author

pbitty commented Aug 23, 2016

Actually, that was simple, just did it now. :)

@shlomi-noach shlomi-noach merged commit 56fd82a into github:master Aug 24, 2016
@shlomi-noach
Copy link
Contributor

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