Fix vtgate_buffer test #5291
Fix vtgate_buffer test #5291
Conversation
Signed-off-by: Anthony Yeh <enisoc@planetscale.com>
Signed-off-by: Anthony Yeh <enisoc@planetscale.com>
| @@ -187,25 +189,34 @@ func syncShardMaster(ctx context.Context, ts *topo.Server, tablet *topodatapb.Ta | |||
| // If active reparents are disabled, we don't touch our MySQL. | |||
| // We just directly update our tablet type to REPLICA. | |||
| func (agent *ActionAgent) abortMasterTerm(ctx context.Context, masterAlias *topodatapb.TabletAlias) error { | |||
There was a problem hiding this comment.
at some point we should rename this to endMasterTerm.
There was a problem hiding this comment.
The name was meant to convey that this is not the "graceful" way for a master term to come to an end. If we hit this code path, it means we should sort of "force demote" ourselves because another master has already taken over behind our backs. Do you think abort is a bad way to describe this?
There was a problem hiding this comment.
That sounds right, but it means that we are ending a master term in a not "graceful" way with the current implementation of PRS. Are we ok with that?
There was a problem hiding this comment.
PRS should have already called DemoteMaster to gracefully step down before we get to this code path. We will still technically execute this code path, but once we make these idempotent, there will be no net effect from this code path in the case of a successful PRS.
|
Oops! Next time I'll make sure to check that it's a |
The main problem was that we now do a full DemoteMaster on the old master, even in the case of TER, as long as
disable_active_reparentis false. This takes a lot longer (because shutting down the queryservice takes 30s before in-progress transactions are cancelled), so the test was timing out at a couple different levels (contexts and waiting for vtgate to find enough healthy tablets).This fixes the test by fixing the timeouts, but I think we should also avoid doing a full DemoteMaster in the case of TER. I'll optimize that in a future PR though. For now, let's at least get the tests green.