-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Remove legacy healthcheck files and structures #10542
Remove legacy healthcheck files and structures #10542
Conversation
Signed-off-by: Florent Poinsard <[email protected]>
f0791e4
to
b6fc805
Compare
Signed-off-by: Florent Poinsard <[email protected]>
b6fc805
to
a809a13
Compare
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
Since this is cleanup, we don't need to back port it to v14. It's a teeny bit risky to refactor this close to release on the release branch. |
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
go/vt/discovery/healthcheck.go
Outdated
TabletRecorder | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we embedding an interface into another? Do we have implementations of TabletRecorder that don't implement HealthCheck?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have implementations of TabletRecorder that don't implement HealthCheck?
No, we don't.
Why are we embedding an interface into another?
This was done so we can use the healthcheck (discovery.HealthCheck
) as a tablet recorder in tx throttler
:
topologyWatcherFactory = func(topoServer *topo.Server, tr discovery.TabletRecorder, cell, keyspace, shard string, refreshInterval time.Duration, topoReadConcurrency int) TopologyWatcherInterface { |
Ultimately, the topo watcher in the discovery
package uses a TabletRecorder
.
Previously, the LegacyHealthCheck
interface was embedding LegacyTabletRecorder
:
vitess/go/vt/discovery/legacy_healthcheck.go
Lines 285 to 289 in d037f24
type LegacyHealthCheck interface { | |
// LegacyTabletRecorder interface adds AddTablet and RemoveTablet methods. | |
// AddTablet adds the tablet, and starts health check on it. | |
// RemoveTablet removes the tablet, and stops its StreamHealth RPC. | |
LegacyTabletRecorder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not have to embed it for things to work.
…hcheck-struct Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
…d target.shard checks Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one change is required - to remove TabletRecorder
from HealthCheck
interface.
Once that is done, this can be merged.
Nice work, and thank you for your patience during review.
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
We decided to remove the |
Description
This Pull Request removes the legacy healthcheck and the related code (legacy topo watcher, legacy replication lag, etc). The different usages have been replaced by the new implementation of the healthcheck. The different files hosting the legacy code have been removed.
Related Issue(s)
legacy_healthcheck.go
and any related structs that are no longer needed #10454Checklist