Skip to content

Use destination master for comparisons during split clone#3450

Closed
tirsen wants to merge 1 commit intovitessio:masterfrom
tirsen:use-dest-master-for-split-clone
Closed

Use destination master for comparisons during split clone#3450
tirsen wants to merge 1 commit intovitessio:masterfrom
tirsen:use-dest-master-for-split-clone

Conversation

@tirsen
Copy link
Copy Markdown
Collaborator

@tirsen tirsen commented Dec 5, 2017

This makes us avoid extra rows which can show up due to replication lag if we're using a replica for comparisons. The master of the destination shard should generally not be loaded since it's not yet taking any traffic.

@tirsen tirsen force-pushed the use-dest-master-for-split-clone branch from 4a25d61 to ed9df10 Compare December 5, 2017 10:00
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.

Are there other uses of this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Only in split clone.

@alainjobart
Copy link
Copy Markdown
Contributor

This is probably better operationally, even though it's not optimal for resource usage. So I agree with this change.

(note there is one case where the destination master could be in use, it's for a reverse vertical split, where a set of tables are moving back to an existing keyspace that has tables. But it's not a very common case, so this should be ok).

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Dec 5, 2017

Tests are failing. It's probably related to the min_healthy thing, because there is only one master.

@tirsen tirsen force-pushed the use-dest-master-for-split-clone branch 5 times, most recently from 9ab76c4 to 2e64c5d Compare February 28, 2018 09:58
@tirsen
Copy link
Copy Markdown
Collaborator Author

tirsen commented Feb 28, 2018

I think the only failure now is that it's flaky. It only happens in the tearDown method.

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Mar 1, 2018

This failure looks real.

@tirsen tirsen force-pushed the use-dest-master-for-split-clone branch from 2e64c5d to deb54e5 Compare March 1, 2018 09:24
@tirsen tirsen force-pushed the use-dest-master-for-split-clone branch 12 times, most recently from 7558407 to d45056b Compare March 6, 2018 13:56
This makes us avoid extra rows which can show up due to replication lag if we're using a replica for comparisons. The master of the destination shard should generally not be loaded since it's not yet taking any traffic.

Signed-off-by: Jon Tirsen <jontirsen@squareup.com>
@tirsen tirsen force-pushed the use-dest-master-for-split-clone branch from d45056b to 8e4de93 Compare March 6, 2018 14:00
@sougou
Copy link
Copy Markdown
Contributor

sougou commented Oct 28, 2018

Can you rebase this and get tests to pass? I think this will also fix @dweitzman's issue: #4284.

deepthi added a commit to planetscale/vitess that referenced this pull request Oct 31, 2018
Signed-off-by: deepthi <deepthi@planetscale.com>
sougou added a commit that referenced this pull request Nov 9, 2018
…split-clone

Rebase #3450 Use destination master for comparison during split clone
@sougou
Copy link
Copy Markdown
Contributor

sougou commented Nov 10, 2018

Merged via #4331.

@sougou sougou closed this Nov 10, 2018
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.

5 participants