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

Use a truly random Destination for DestinationAnyShard #4522

Merged

Conversation

eeSeeGee
Copy link
Contributor

Signed-off-by: Aaron Young [email protected]

@eeSeeGee eeSeeGee requested a review from sougou as a code owner January 14, 2019 22:27
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.

LGTM

@rafael
Copy link
Member

rafael commented Jan 14, 2019

This is great! We've noticed in this in the past and wanted to fix it

@rafael
Copy link
Member

rafael commented Jan 14, 2019

However, tests are going to fail because they are expecting to be in shard one. One approach that we discussed to address this is that the shard "picker" should be an interface. That way we can use random in the real one, but a mocked one that always picks the first shard that is used in the context of the tests.

@xhh1989
Copy link
Contributor

xhh1989 commented Jan 15, 2019

We JDer use this function for pined table that pined to the first shard.

@eeSeeGee eeSeeGee force-pushed the young.20190114.random_any_destination branch from 2f123e8 to 87a9adf Compare January 15, 2019 05:20
@eeSeeGee
Copy link
Contributor Author

We JDer use this function for pined table that pined to the first shard.

That's... weird? Why not use a separate keyspace?

@sougou
Copy link
Contributor

sougou commented Jan 15, 2019

We JDer use this function for pined table that pined to the first shard.

That's... weird? Why not use a separate keyspace?

Do you have this code in your branch: https://github.com/vitessio/vitess/blob/master/go/vt/vtgate/planbuilder/from.go#L193. If so, pinned tables should be routed to the shards they belong to.

@sougou
Copy link
Contributor

sougou commented Jan 16, 2019

@xhh1989 Can you confirm that you have the code I pointed at? If so, this change should not affect you.

@xhh1989
Copy link
Contributor

xhh1989 commented Jan 17, 2019

we have a plan upgrade to version 3.0, there a big difference between JD version and 3.0, we add a single shard table feature based on version 2.1 and that single shard table like pined table but pined on the first shard, use DestinationAnyShard for routing the first shard. @sougou you can merge the PR, I won't let this PR affect our internal version upgrade.

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