tm revamp: deprecate obsolete code #6241
Conversation
| @@ -94,6 +94,7 @@ func (agent *ActionAgent) changeTypeLocked(ctx context.Context, tabletType topod | |||
| } | |||
|
|
|||
| // let's update our internal state (stop query service and other things) | |||
| // refreshTablet will runHealthCheck. | |||
There was a problem hiding this comment.
Is this true? I don't see where it happens.
There was a problem hiding this comment.
I think this comment is incorrect. I likely meant broadcastHealth, which is needed to inform the subscribers of the type change. The rest of runHealthCheck seems redundant because refreshState does that part of the work, which is to start/stop services as needed.
I'll change this comment, but I'm now worried that I might have missed something. Do you see something obvious?
There was a problem hiding this comment.
I can see that runHealthCheck and refreshState do similar things with start/stop services, but it is not immediately obvious whether they will both come to the same conclusion in all situations. In the old sequence of actions, refreshState would leave some breadcrumbs (disallowQueryReason) that runHealthCheck took into account.
It does seem simpler and cleaner to not call runHealthCheck here.
There was a problem hiding this comment.
These are idempotent functions that make the system eventually converge. Many places have this call because "there's no harm". The harmful part of it is that there is no precision or clear knowledge of what state we are in and what should be done.
In this particular situation, calling the function prematurely caused a health check failure, which introduced delays that previously did not exist. I'm hoping to iterate on this to bring more clarity on what should happen when and why.
In principle, the idempotence approach is a good one, but probably not in this situation. Where I think this would be useful is synchronizing the tablet state against mysql itself.
There was a problem hiding this comment.
I'm convinced. LGTM after the comment is changed to broadcastHealth().
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
It was making direct updates to the tablet record. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
ChangeType was invoking a runHealthCheck at the end, but this is unnecessary because the refreshTablet call already runs it. I found it while changing tests that were using the old TER to use the new ChangeType calls. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
c1fa9e5 to
740c92d
Compare
Deprecating old TER and schemaswap.