Skip to content

Add startup argument to stop tablet from reparent#5203

Merged
sougou merged 2 commits intovitessio:masterfrom
systay:never-master
Oct 5, 2019
Merged

Add startup argument to stop tablet from reparent#5203
sougou merged 2 commits intovitessio:masterfrom
systay:never-master

Conversation

@systay
Copy link
Collaborator

@systay systay commented Sep 19, 2019

No description provided.

Signed-off-by: Andres Taylor <antaylor@squareup.com>
Signed-off-by: Andres Taylor <antaylor@squareup.com>
@deepthi
Copy link
Collaborator

deepthi commented Sep 19, 2019

how is this different from -disable_active_reparents?
cc @enisoc

@sougou
Copy link
Contributor

sougou commented Sep 19, 2019

This looks like it's trying to protect from an external tool's error where it should not have reparented to a certain tablet.

My initial reaction is that this is an organic addition meant to handle a specific corner case. It may not necessarily play well with other operations like PlannedReparentShard, etc. We've been previously burnt by one-off flags like this, and are still stuck with many of them.

We should think of a more holistic design and approach that uniformly applies to all reparenting operations. What should that be? I don't know yet. I'll respond once I've had more time to think.

@enisoc
Copy link
Member

enisoc commented Sep 20, 2019

@systay Can you elaborate on when you would want to run a tablet in this mode?

At face value, I agree with @sougou that this doesn't seem like a good place to put this logic. TER is meant to inform Vitess that the mysql instance under that tablet is already the master. It doesn't seem like the tablet's place to refuse to respect that command, since the contract of TER is that whatever system made that mysql instance the master should be trusted as the source of truth.

@tirsen
Copy link
Collaborator

tirsen commented Sep 23, 2019

This is when you run a tablet connected to an Aurora reader endpoint. That tablet should never become a MASTER. This is indeed sort of a workaround for a for a bug in an external tool.

@enisoc
Copy link
Member

enisoc commented Sep 24, 2019

This is when you run a tablet connected to an Aurora reader endpoint. That tablet should never become a MASTER.

We could generalize this to a pattern that I've been thinking we'll need soon anyway, when we make Vitess handle automatic failover on its own. The general principle is that we need to stop assuming all REPLICA tablets are equally eligible to become masters. Up to now, REPLICA as a category has been used to denote two properties that are actually orthogonal: 1) the tablet should serve OLTP read queries, but not OLAP reads, and 2) the tablet is eligible to become a MASTER.

I do think we need to have a way to specify that some tablets serve OLTP reads, but are not master-eligible. However, I agree with @sougou that we should do this in a way that works with PlannedReparentShard and other existing commands, which perhaps means it should be stored in topology rather than only a vttablet flag.

We will also probably need a way to specify precedence among those tablets that are master-eligible (e.g. maybe you want to increase the master-preference of tablets in certain cells), but that's beyond the scope of this discussion. I only mention it to make the point that this is all the same type of stuff that Orchestrator supports, and those features have been useful, so we'll need something like that if we want to make Vitess handle everything without Orc. The other point is that we could take inspiration from how Orc expresses these kinds of preferences.

@tirsen
Copy link
Collaborator

tirsen commented Oct 1, 2019

I agree completely about the muddling of the two orthogonal concerns in the REPLICA tablet type. Looking forward to the refactor. In the meantime though this flag closes up a hole that has caused us production outages. Should we just merge this to our fork instead?

@enisoc
Copy link
Member

enisoc commented Oct 1, 2019

Should we just merge this to our fork instead?

That would be my personal preference for cases like this. @sougou WDYT?

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.

I can't say I like this. But fine.

@sougou sougou merged commit 2731866 into vitessio:master Oct 5, 2019
@deepthi
Copy link
Collaborator

deepthi commented Oct 6, 2019

I thought this was going to stay in square's fork and not be merged to master?

@sougou
Copy link
Contributor

sougou commented Oct 6, 2019

ohh. I misunderstood...
Let me revert.

sougou added a commit to planetscale/vitess that referenced this pull request Oct 6, 2019
This reverts commit 2731866, reversing
changes made to 698935b.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
systay pushed a commit that referenced this pull request Oct 7, 2019
Revert "Merge pull request #5203 from systay/never-master"
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