Skip to content

Adding code to properly split_clone a v3 table.#1634

Merged
alainjobart merged 6 commits into
vitessio:masterfrom
alainjobart:v3resharding
Apr 12, 2016
Merged

Adding code to properly split_clone a v3 table.#1634
alainjobart merged 6 commits into
vitessio:masterfrom
alainjobart:v3resharding

Conversation

@alainjobart
Copy link
Copy Markdown
Contributor

We need a vindex of cost < 2 on a table (that is numeric or hash).
Then we use it to split the rows.

@sougou you can take a look if you want, but I'm gonna keep working on it to add a few more things there.


This change is Reviewable

We need a vindex of cost < 2 on a table (that is numeric or hash).
Then we use it to split the rows.
@sougou
Copy link
Copy Markdown
Contributor

sougou commented Apr 12, 2016

Actually, we should just go for the primary vindex (colvindexes[0]), because it will most likely be the sharding key (lowest cost). It's also guaranteed to be unique, which is another requirement.

scw.keyspace: kformal,
},
}
vschema, err := vindexes.BuildVSchema(formal)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you already noticed. This code is very similar to vindexes.Validate. We could look at sharing.

I see the rest of the stuff is WIP. I'll comment as things develop.

It turns out it's the most likely one, no need to scan them all.
Building the formal vschema for a keyspace was shared in a couple
places.
The test was returning all kinds of rows, had to fix that, because now
the filtering can be done at the app layer for v3.
Also, the test was describing views in the test schema, which is not
right, as the code doesn't ask for them.
We used to think we should run one diff per destination shard. Now we
assume we want one diff for each (source, destination) tuple. That way
splits and merges will work the same way, and we never need merge-sort.

In the process, fix the keyrange overlap usage to work for both splits
and merges.
@alainjobart
Copy link
Copy Markdown
Contributor Author

@sougou this is ready for review.
@michael-berlin PTAL at the last commit, that's the change we just talked about. It's much simpler. May require some more parameters in automation framework.

if colVindex.Vindex.Cost() > 1 {
return nil, fmt.Errorf("primary vindex cost is too high for table %v", td.Name)
}
unique, ok := colVindex.Vindex.(vindexes.Unique)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: use vindexes.IsUnique instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahh. Nvm. Looks like you need the Unique interface var also.

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Apr 12, 2016

LGTM

Approved with PullApprove

@alainjobart alainjobart merged commit 6519d59 into vitessio:master Apr 12, 2016
@alainjobart alainjobart deleted the v3resharding branch April 12, 2016 22:27
@michael-berlin
Copy link
Copy Markdown
Contributor

LGTM

Some minor comments. Feel free to fix them forward if you like.


Reviewed 4 of 4 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, 2 of 2 files at r4, 5 of 5 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 8 unresolved discussions.


go/vt/worker/diff_utils.go, line 84 [r5] (raw file):
nit: Missing period at the end.


go/vt/worker/split_diff_cmd.go, line 66 [r6] (raw file):
optional: You could add the default to worker/defaults.go and reference it here.


go/vt/worker/split_diff_cmd.go, line 172 [r6] (raw file):
optional: You could add the default to worker/defaults.go and reference it here.


go/vt/worker/split_diff_test.go, line 287 [r5] (raw file):
nit: half


go/vt/worker/split_diff_test.go, line 288 [r5] (raw file):
nit: SQL


Comments from Reviewable

Approved with PullApprove

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