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.

Sign up for GitHub

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

Jump to bottom

Create Region JSON Vindex #5806

Merged
merged 19 commits into from Mar 20, 2020
Merged

Create Region JSON Vindex #5806

merged 19 commits into from Mar 20, 2020

Conversation

RyanLeonardSpruce
Copy link
Contributor

Adds the region_vindex (as region_json) created for a Vitess conference demo (https://github.com/planetscale/vitess/blob/ds-kcn19-demo/go/vt/vtgate/vindexes/region_vindex.go), with modifications so it works with the latest Vitess version.

sougou
sougou reviewed Feb 7, 2020
View reviewed changes
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Couple of nits.

@deepthi can you also take a look?

go/vt/vtgate/vindexes/region_json.go Outdated Show resolved Hide resolved
go/vt/vtgate/vindexes/region_json.go Outdated Show resolved Hide resolved
@sougou sougou requested a review from deepthi February 7, 2020 18:07
deepthi
deepthi reviewed Feb 7, 2020
View reviewed changes
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.

It will be nice to come up with an alternative to JSON, but I understand that is not the scope of this PR.

go/vt/vtgate/vindexes/region_json.go Outdated Show resolved Hide resolved
go/vt/vtgate/vindexes/region_json.go Outdated Show resolved Hide resolved
go/vt/vtgate/vindexes/region_json.go Show resolved Hide resolved
@RyanLeonardSpruce
Added (broken) region Vindex from https://github.com/planetscale/vite…
5579aed 
…ss/tree/ds-kcn19-demo

Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Implemented changes similar to a091391#diff-8ddd8af3d810b9a1c2bf9650e…
8266a34 
…bed76e7

Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Implemented changes similar to c8ccf37#diff-8ddd8af3d810b9a1c2bf9650e…
c9369bc 
…bed76e7

Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Added regionBytes field.
27dab6b 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Removed extra import. NeedVCursor -> NeedsVCursor
d834206 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Copied 'Verify' from a091391#diff-8ddd8af3d810b9a1c2bf9650ebed76e7
6571605 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Import bytes.
dde3a3f 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
bytes -> byte
99c3b2b 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Updated formatting.
d9fe561 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Renamed 'region_vindex' -> 'region_json'.
c73e8d4 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Updated copyright, removed log messages.
57a37ef 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Added line of code accidentally deleted with logs.
54189ea 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Typo staisfies -> satisfies, nit: region_experimental -> RegionExperi…
c8e7adc 
…mental

Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Added 'RegionExperimental' wrapping back. Removed 'Verify' function.
df13dcb 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Fixed formatting.
392c92a 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Removed unused import.
57a2bad 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Revert "Added 'RegionExperimental' wrapping back. Removed 'Verify' fu…
4fb7697 
…nction."

This reverts commit da1853d9a1ac0da9aec2c47884b80b8d57a53c2d.

Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@RyanLeonardSpruce
Added bytes back in.
23fe963 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
sougou
sougou reviewed Feb 12, 2020
View reviewed changes
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Change is good. There's one minor nit.

go/vt/vtgate/vindexes/region_json.go Outdated
// RegionMap is used to store mapping of country to region
type RegionMap map[string]uint64

// RegionJson defines a vindex that uses a lookup table.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is outdated. Looks like a copy-pasta from RegionExperimental which should also be fixed.

For RegionJson, I'd recommend:
RegionJson is a multi-column unique vindex. The first column is used to lookup the prefix part of the keyspace id, the second column is hashed, and the two values are combined to produce the keyspace id. RegionJson can be used for geo-partitioning because the first column can denote a region, and it will dictate the shard range for that region.

For RegionExperimental, it should be as above except that the first column is itself the prefix of the keyspace id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sougou I've changed the RegionJson description. Did you want me to also change the RegionExperimental description in the PR?

Copy link
Member

Choose a reason for hiding this comment

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

If you don't mind, that will be great.

@RyanLeonardSpruce RyanLeonardSpruce mentioned this pull request Feb 27, 2020
Closed
@RyanLeonardSpruce
Updated RegionJson description with comments from @sougou
ddbabb1 
Signed-off-by: Ryan Leonard <rleonard@spruce.com>
@sougou sougou merged commit 25e6fa1 into vitessio:master Mar 20, 2020
@deepthi deepthi mentioned this pull request Mar 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepthi deepthi deepthi left review comments

@sougou sougou sougou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RyanLeonardSpruce @deepthi @sougou