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

Online DDL/VReplication: able to read from replica #8405

Merged

Conversation

shlomi-noach
Copy link
Contributor

Description

Very quick PR, that enables VReplication/OnlineDDL reading from REPLICA (fallback to MASTER if REPLICA is not available). This means vstream source can be a replica, and vreplication's grand SELECT will not put the primary at risk.

Related Issue(s)

#6926

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

@shlomi-noach shlomi-noach added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving labels Jun 30, 2021
@rohit-nayak-ps
Copy link
Contributor

The tablettypes parameter is treated as an unordered list. So the tablet picker will chose randomly between the replicas available and the primary, if REPLICA,MASTER is used. Thus this PR will not solve reliably fix the "grand SELECT" problem.

@rohit-nayak-ps
Copy link
Contributor

@deepthi, should we default to choosing other tablet types first if they are specified in addition to MASTER. It will technically be breaking compatibility, but I feel that would be the preferred default for most use cases? We could, of course, add an additional FALLBACK-PRIMARY or some such option to enable this logic.

@shlomi-noach
Copy link
Contributor Author

shlomi-noach commented Jul 8, 2021

It will technically be breaking compatibility,

I'm not sure. Is the current order defined as random or is it undefined? If it's undefined then we don't break compatibility if we choose to prioritize REPLICA over MASTER.

@rohit-nayak-ps
Copy link
Contributor

It will technically be breaking compatibility,

I'm not sure. Is the current order defined as random or is it undefined? If it's undefined then we don't break compatibility if we choose to prioritize REPLICA over MASTER.

Not sure if we have a formal spec for it, so undefined I guess. I was talking of a subtle expectation like if there is just one REPLICA and one MASTER then with the current implementation load for multiple streams will be split 50/50 across the two, but if we change it then it will be 100% on the REPLICA.

But I think that is a minor quibble and we should be fine with prioritizing. @aquarapid, I would be interested in hearing your opinion on this.

@shlomi-noach
Copy link
Contributor Author

with the current implementation load for multiple streams will be split 50/50 across the two, but if we change it then it will be 100% on the REPLICA.

No, you're right. That's a massive kind of change.

We can go the DSL way, and support something like REPLICA>MASTER which prioritizes one over the other. Or, as you suggested, a REPLICA-MASTER or FALLBACK-MASTER entry which encapsulates a prioritization logic.

@shlomi-noach
Copy link
Contributor Author

I added a hint, "in_order:" which sugests in what order tablets should be picked.

When the hint is not provided, behavior remains the same as today (randomly pick from all available tablets, exhaust list)

When the hint is provided, for example "in_order:REPLICA,MASTER", then:

  • Tablet picker will first attempt to return a REPLICA, in random order for all available REPLICA tablets
  • Any only if all REPLICAs are exhausted and cannot serve, then it picks a MASTER.

The logic for both in-order and random order is consolidated, and it's down to how we sort or shuffle the candidates slice.

Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

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

nice
lgtm

@shlomi-noach shlomi-noach merged commit 827d312 into vitessio:main Jul 12, 2021
@shlomi-noach shlomi-noach deleted the onlineddl-vrepl-replica-tablet branch July 12, 2021 04:18
DeathBorn added a commit to vinted/vitess that referenced this pull request Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants