DeleteTablet: allow deletion of old master tablet without -allow_master #5647
Conversation
…er flag Signed-off-by: deepthi <deepthi@planetscale.com>
go/vt/wrangler/tablet_test.go
Outdated
| Type: topodatapb.TabletType_MASTER, | ||
| } | ||
|
|
||
| if err := wr.InitTablet(context.Background(), tablet, false /*allowMasterOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/); err != nil { |
There was a problem hiding this comment.
I'm a bit confused how this test is working. Shouldn't init tablet fail here if there is already a master tablet?
vitess/go/vt/wrangler/tablet.go
Line 71 in 510b1fe
There was a problem hiding this comment.
si.HasMaster() will be false (si.MasterAlias is nil until it is set).
There was a problem hiding this comment.
I have modified the test case to more closely reflect the real-life use case.
There was a problem hiding this comment.
I think it will be better to move the topotools.DeleteTablet step to after the wasMaster step. It will make the operation idempotent. Otherwise, if the wasMaster step fails, we cannot retry the Delete because the tablet record would have been deleted already.
Good point. I will make this change. |
go/vt/wrangler/tablet.go
Outdated
| @@ -109,7 +109,13 @@ func (wr *Wrangler) DeleteTablet(ctx context.Context, tabletAlias *topodatapb.Ta | |||
| if err != nil { | |||
| return err | |||
| } | |||
| wasMaster := ti.Type == topodatapb.TabletType_MASTER | |||
| si, err := wr.ts.GetShard(ctx, ti.Keyspace, ti.Shard) | |||
There was a problem hiding this comment.
We should avoid depending on global topo being up whenever possible. In the common case where we're deleting a tablet whose record doesn't claim to be master, I think we should not bother checking the shard record.
go/vt/wrangler/tablet.go
Outdated
| } | ||
|
|
||
| // the true master tablet will have the same MasterTermStartTime as the shard | ||
| wasMaster := ti.Type == topodatapb.TabletType_MASTER && topoproto.TabletAliasEqual(si.MasterAlias, tabletAlias) && logutil.ProtoToTime(ti.MasterTermStartTime).Equal(logutil.ProtoToTime(si.MasterTermStartTime)) |
There was a problem hiding this comment.
I think we should be a bit more tolerant of inconsistency. There are various reasons that the timestamps in the tablet and shard records might not match. For example, perhaps the tablet has started a new term but hasn't updated the shard record yet.
Here's the way I think of it:
- If the tablet record claims to be non-master, believe it without checking the shard record.
- Unless we find out there's a problematic case of inconsistency here, we should err on the side of keeping dependence on global topo as low as possible. The specific case we're trying to fix now is only when a tablet record does claim to be master.
- If the tablet record claims to be master, check the shard record to confirm.
- If the shard record has this tablet as master, assume it is master without checking timestamps.
- If the shard record has another tablet as master, check the timestamps.
- If the shard record's master timestamp is newer, assume this tablet is not the master.
- If the tablet record's mater timestamp is newer, assume this tablet is the master.
There was a problem hiding this comment.
I agree with being more tolerant of inconsistency. However, I'm on the fence about minimizing global topology dependency in this context. Here my rationale for this:
- This is an admin operation explicitly triggered by an operator, I don't feel that strongly about minimal global topology footprint in those contexts. I would also feel different if this were an operation that could leave a serving tablet in a non serving state.
- Given that there is still a case where you have to check the shard record, I would lean towards keeping the code as simple as possible.
There was a problem hiding this comment.
Dependence on global topo is tough to eliminate because it can pop up anywhere in a distributed call stack and cause the operation you were attempting to fail when global topo is unreachable. If we let more unnecessary cases creep in, it's only increasing the work we'll have to do later to meet our long-term goal of reducing dependence on global topo.
I think we can still keep the code simple while only talking to global topo when necessary. For example, if we break the logic out into a new function to answer "is this tablet potentially the master?", this function will remain clean, and we can even use that in other places to apply similar corrections where we're still trusting the tablet record alone.
There was a problem hiding this comment.
Yeah don't take me wrong. I also agree and have been working on reducing dependency in global topo. Just that benefit is not obvious in the case, given that is not really removing it.
If we can keep it simple and clean, I'm on board. My high level concern was to not add complexity when the actual dependency is still there.
There was a problem hiding this comment.
I would argue the extra complexity is modest and worthwhile to avoid increasing the dependency more than necessary. The kind of dependency I mean is not, "Does this code ever call global topo?" but rather, "What set of actions can I take while global topo is unavailable?"
Although this code will still contain a call to global topo either way, putting the call inside a conditional branch means we get to keep "delete non-master tablets" in the set of actions you can take while global topo is unavailable.
There was a problem hiding this comment.
With that said, I also don't feel strongly about this. I'll side with whatever @deepthi thinks is the right balance.
…define shard master. Reduce dependence on global topo. Improve test case Signed-off-by: deepthi <deepthi@planetscale.com>
Signed-off-by: deepthi <deepthi@planetscale.com>
Signed-off-by: deepthi <deepthi@planetscale.com>
…ke-master DeleteTablet: allow deletion of old master tablet without -allow_master
The way we define the true master in DeleteTablet is now consistent with how we define a shard's master, taking into account the shard's MasterAlias and MasterTermStartTime.
Signed-off-by: deepthi deepthi@planetscale.com