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

Change semi-sync information to use the parameter passed and deprecate enable_semi_sync #9725

Merged
merged 7 commits into from
Feb 25, 2022

Conversation

GuptaManan100
Copy link
Member

@GuptaManan100 GuptaManan100 commented Feb 17, 2022

Description

This PR deprecates enable_semi_sync flag and changes the implementation for fixSemiSync to use the flag passed down from the RPC. Backup and RestoreFromBackup RPC's are also fixed to call SetReplicationSource when they finish. This is needed since the primary could have changed during the backup or restore process and the semi-sync settings need to be updated.

Related Issue(s)

#8975

Checklist

Deployment Notes

enable_semi_sync is deprecated and the user should be setting the correct durability_policy instead.

@GuptaManan100 GuptaManan100 added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Cluster management release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) Type: Dependencies Dependency updates labels Feb 17, 2022
@GuptaManan100 GuptaManan100 marked this pull request as draft February 17, 2022 15:28
… using enable_semi_sync

Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100 GuptaManan100 marked this pull request as ready for review February 23, 2022 00:47
Signed-off-by: Manan Gupta <manan@planetscale.com>
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I had only one question.
Nice work!

// In SetReplicationSource, we find that the source host and port was already set correctly,
// So we try to stop and start replication. The first STOP SLAVE comes from there
"STOP SLAVE",
// During the START SLAVE call, we find a relay log error, so we try to restart replication.
Copy link
Member

Choose a reason for hiding this comment

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

Is the relay log error from START or STOP?

Copy link
Member Author

Choose a reason for hiding this comment

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

We get it from StartReplicaton command to mysql. In our test environment we are using the FakeMySQLDaemon

// StartReplication is part of the MysqlDaemon interface.
func (fmd *FakeMysqlDaemon) StartReplication(hookExtraEnv map[string]string) error {
	if fmd.StartReplicationError != nil {
		return fmd.StartReplicationError
	}
	return fmd.ExecuteSuperQueryList(context.Background(), []string{
		"START SLAVE",
	})
}

and we set the StartReplicationError as -

goodReplica1.FakeMysqlDaemon.StartReplicationError = errors.New("Slave failed to initialize relay log info structure from the repository")

So we get the error in StartReplication, but it isn't seen in the Execute Super Query List

@GuptaManan100 GuptaManan100 merged commit ed8aa8f into vitessio:main Feb 25, 2022
@GuptaManan100 GuptaManan100 deleted the semi-sync-cutover branch February 25, 2022 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Cluster management release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants