Skip to content

resharding: auto-reverse replication#4262

Merged
sougou merged 1 commit intovitessio:masterfrom
planetscale:ss-resharding
Oct 20, 2018
Merged

resharding: auto-reverse replication#4262
sougou merged 1 commit intovitessio:masterfrom
planetscale:ss-resharding

Conversation

@sougou
Copy link
Copy Markdown
Contributor

@sougou sougou commented Oct 10, 2018

MigrateServedTypes for master automatically does the prep work for
reversing replication. If the flag is specified, it reverses the
direction thereby allowing you to migrate back to the original
masters.

If the flag was not originally specified, then it only does the prep
work, without actually starting the replication. You can later
reissue the Migrate command, which will then start the vreplication.

Signed-off-by: Sugu Sougoumarane ssougou@gmail.com

MigrateServedTypes for master automatically does the prep work for
reversing replication. If the flag is specified, it reverses the
direction thereby allowing you to migrate back to the original
masters.

If the flag was not originally specified, then it only does the prep
work, without actually starting the replication. You can later
reissue the Migrate command, which will then start the vreplication.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
@sougou sougou requested review from demmer and rafael October 10, 2018 14:39
@tirsen
Copy link
Copy Markdown
Collaborator

tirsen commented Oct 10, 2018

WHOA!

reverse := subFlags.Bool("reverse", false, "Moves the served tablet type backward instead of forward. Use in case of trouble")
skipReFreshState := subFlags.Bool("skip-refresh-state", false, "Skips refreshing the state of the source tablets after the migration, meaning that the refresh will need to be done manually, replica and rdonly only)")
filteredReplicationWaitTime := subFlags.Duration("filtered_replication_wait_time", 30*time.Second, "Specifies the maximum time to wait, in seconds, for filtered replication to catch up on master migrations")
reverseReplication := subFlags.Bool("reverse_replication", false, "For master migration, enabling this flag reverses replication allowing which allows you to rollback")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove "allowing"


// Phase 2
// Always setup reverse replication. We'll start it later if reverseReplication was specified.
// This will allow someone to reverse the replication later if they change their mind.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah so even if you don't pass the reverse_replication flag you can still start vreplication and revert the migration?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In fact, you should't start vreplication yourself. You should just reissue MigrateServedTypes with -reverse_replication.

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Oct 18, 2018

@demmer @tirsen Need your thoughts on something @rafael and I have been debating.

The current implementation of this feature is as follows:

  • Create metadata for reversing replication
  • Perform the cutover
  • If reversReplication { startReverseReplication() }

@rafael proposes we change this to:

  • if reverseReplication{ createMetadata() }
  • Perform the cutover
  • If reverseReplication { startReverseReplication() }

The current approach:
Pro: If you Migrate and forgot to specify -reverse_replication, you can invoke it again with the flag and reverse it later.
Con: It creates additional metadata you may not use
Con: It's confusing

@rafael proposal:
Pro: System does not do additional work without the flag.
Pro: More explicit, and less confusing
Con: You can't reverse replication later if you change your mind.

Let us know if you have any preferences.

@tirsen
Copy link
Copy Markdown
Collaborator

tirsen commented Oct 19, 2018

I think the safest option should be the default but this is a somewhat unproven option so maybe as a first step make it not the default and then we make it the default once it's been well tested in the field.

@tirsen
Copy link
Copy Markdown
Collaborator

tirsen commented Oct 19, 2018

Seems crazy not to save the metadata though.

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Oct 19, 2018

Clarified with @tirsen: auto-reverse should not happen unless requested, but metadata should be created irrespective of the flag. This is the current implementation.

@demmer
Copy link
Copy Markdown
Member

demmer commented Oct 19, 2018

My preference would be somewhere in between...

Could we unconditionally save the master position metadata that you would need to start reverse replication, but don't set up the replication itself unless requested? It could be a new workflow type of "SavedReverseReplicationPos" or some such which would never actually be used to start the replication, but from which we could get the positions if needed.

@tirsen
Copy link
Copy Markdown
Collaborator

tirsen commented Oct 20, 2018

We don't currently use workflows so we wouldn't benefit from saving stuff there.

In my opinion safest option should always be the default. Based on that philosophy by default we should even start the reverse vreplication. That said, this is still a somewhat unproven feature so might need to see some more field testing first. As a first step setting up the vreplication but not starting it seems like a safe low risk option.

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Oct 20, 2018

Per discussion so far with @demmer, he's leaning towards @rafael's approach, which is to not setup reverse replication if it was not requested.

It's a tie. But since I'm not a user, I'll count my vote as the weakest. I'm still trying to make a case for this. However, if I don't succeed, the decision will be to not provide the possibility of later reversing replication if it was not requested at the time of issuing the Migrate.

If so, the third option suggested by @demmer can be considered for the future.

@rafael
Copy link
Copy Markdown
Member

rafael commented Oct 20, 2018

@demmer idea makes sense. My main point with the discussion with @sougou is that current API is flexible but confusing from an user perspective. I don't think is worth it to get stuck on that point though.

I'm supportive of moving on and do something like @demmer is suggesting in a future PR.

@sougou sougou merged commit 50960f6 into vitessio:master Oct 20, 2018
@tirsen
Copy link
Copy Markdown
Collaborator

tirsen commented Oct 22, 2018

For posterity, which approach did we end up merging? (Too lazy to read the code.)

3 similar comments
@tirsen
Copy link
Copy Markdown
Collaborator

tirsen commented Oct 22, 2018

For posterity, which approach did we end up merging? (Too lazy to read the code.)

@tirsen
Copy link
Copy Markdown
Collaborator

tirsen commented Oct 22, 2018

For posterity, which approach did we end up merging? (Too lazy to read the code.)

@tirsen
Copy link
Copy Markdown
Collaborator

tirsen commented Oct 22, 2018

For posterity, which approach did we end up merging? (Too lazy to read the code.)

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Oct 22, 2018

Always save metadata. Initiate auto-reverse only if requested.

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.

4 participants