Skip to content
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

Merged
merged 17 commits into from
Jul 8, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
238 changes: 0 additions & 238 deletions go/vt/discovery/fake_legacy_healthcheck.go

This file was deleted.

41 changes: 39 additions & 2 deletions go/vt/discovery/healthcheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import (
"time"

"vitess.io/vitess/go/flagutil"
"vitess.io/vitess/go/netutil"
"vitess.io/vitess/go/stats"
"vitess.io/vitess/go/vt/log"
"vitess.io/vitess/go/vt/proto/query"
Expand Down Expand Up @@ -81,12 +82,15 @@ var (
refreshKnownTablets = flag.Bool("tablet_refresh_known_tablets", true, "tablet refresh reloads the tablet address/port map from topo in case it changes")
// topoReadConcurrency tells us how many topo reads are allowed in parallel
topoReadConcurrency = flag.Int("topo_read_concurrency", 32, "concurrent topo reads")

// How much to sleep between each check.
waitAvailableTabletInterval = 100 * time.Millisecond
)

// See the documentation for NewHealthCheck below for an explanation of these parameters.
const (
defaultHealthCheckRetryDelay = 5 * time.Second
defaultHealthCheckTimeout = 1 * time.Minute
DefaultHealthCheckRetryDelay = 5 * time.Second
DefaultHealthCheckTimeout = 1 * time.Minute
deepthi marked this conversation as resolved.
Show resolved Hide resolved

// DefaultTopoReadConcurrency is used as the default value for the topoReadConcurrency parameter of a TopologyWatcher.
DefaultTopoReadConcurrency int = 5
Expand Down Expand Up @@ -166,6 +170,8 @@ type tabletAliasString string

// HealthCheck declares what the TabletGateway needs from the HealthCheck
type HealthCheck interface {
TabletRecorder

Copy link
Member

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?

Copy link
Member Author

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:

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

Copy link
Member

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.

// CacheStatus returns a displayable version of the health check cache.
CacheStatus() TabletsCacheStatusList

Expand Down Expand Up @@ -625,6 +631,25 @@ func (hc *HealthCheckImpl) GetHealthyTabletStats(target *query.Target) []*Tablet
return append(result, hc.healthy[KeyFromTarget(target)]...)
}

// GetHealthyTabletStats returns only the healthy tablets.
systay marked this conversation as resolved.
Show resolved Hide resolved
// The returned array is owned by the caller.
// For TabletType_PRIMARY, this will only return at most one entry,
// the most recent tablet of type primary.
// This returns a copy of the data so that callers can access without
// synchronization
func (hc *HealthCheckImpl) GetTabletStats(target *query.Target) []*TabletHealth {
var result []*TabletHealth
hc.mu.Lock()
defer hc.mu.Unlock()
if target.Shard == "" {
target.Shard = "0"
}
deepthi marked this conversation as resolved.
Show resolved Hide resolved
for _, health := range hc.healthData[KeyFromTarget(target)] {
result = append(result, health)
}
return result
}

// getTabletStats returns all tablets for the given target.
// The returned array is owned by the caller.
// For TabletType_PRIMARY, this will only return at most one entry,
Expand Down Expand Up @@ -899,3 +924,15 @@ func (hc *HealthCheckImpl) stateChecksum() int64 {

return int64(crc32.ChecksumIEEE(buf.Bytes()))
}

// TabletToMapKey creates a key to the map from tablet's host and ports.
// It should only be used in discovery and related module.
func TabletToMapKey(tablet *topodata.Tablet) string {
parts := make([]string, 0, 1)
for name, port := range tablet.PortMap {
parts = append(parts, netutil.JoinHostPort(name, port))
}
sort.Strings(parts)
parts = append([]string{tablet.Hostname}, parts...)
return strings.Join(parts, ",")
}
23 changes: 16 additions & 7 deletions go/vt/discovery/healthcheck_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,22 @@ import (
topodatapb "vitess.io/vitess/go/vt/proto/topodata"
)

var connMap map[string]*fakeConn
var connMapMu sync.Mutex
var (
connMap map[string]*fakeConn
connMapMu sync.Mutex
)

func testChecksum(t *testing.T, want, got int64) {
t.Helper()
if want != got {
t.Errorf("want checksum %v, got %v", want, got)
}
}

func init() {
tabletconn.RegisterDialer("fake_gateway", tabletDialer)

//log error
// log error
if err := flag.Set("tablet_protocol", "fake_gateway"); err != nil {
log.Errorf("failed to set flag \"tablet_protocol\" to \"fake_gateway\":%v", err)
}
Expand Down Expand Up @@ -196,7 +205,7 @@ func TestHealthCheck(t *testing.T) {
}
input <- shr
result = <-resultChan
//TODO: figure out how to compare objects that contain errors using utils.MustMatch
// TODO: figure out how to compare objects that contain errors using utils.MustMatch
assert.True(t, want.DeepEqual(result), "Wrong TabletHealth data\n Expected: %v\n Actual: %v", want, result)
testChecksum(t, 1027934207, hc.stateChecksum()) // unchanged

Expand Down Expand Up @@ -257,7 +266,7 @@ func TestHealthCheckStreamError(t *testing.T) {
LastError: fmt.Errorf("some stream error"),
}
result = <-resultChan
//TODO: figure out how to compare objects that contain errors using utils.MustMatch
// TODO: figure out how to compare objects that contain errors using utils.MustMatch
assert.True(t, want.DeepEqual(result), "Wrong TabletHealth data\n Expected: %v\n Actual: %v", want, result)
// tablet should be removed from healthy list
a := hc.GetHealthyTabletStats(&querypb.Target{Keyspace: "k", Shard: "s", TabletType: topodatapb.TabletType_REPLICA})
Expand Down Expand Up @@ -317,7 +326,7 @@ func TestHealthCheckErrorOnPrimary(t *testing.T) {
LastError: fmt.Errorf("some stream error"),
}
result = <-resultChan
//TODO: figure out how to compare objects that contain errors using utils.MustMatch
// TODO: figure out how to compare objects that contain errors using utils.MustMatch
assert.True(t, want.DeepEqual(result), "Wrong TabletHealth data\n Expected: %v\n Actual: %v", want, result)
// tablet should be removed from healthy list
a := hc.GetHealthyTabletStats(&querypb.Target{Keyspace: "k", Shard: "s", TabletType: topodatapb.TabletType_PRIMARY})
Expand Down Expand Up @@ -1158,7 +1167,7 @@ func TestTemplate(t *testing.T) {
}

func TestDebugURLFormatting(t *testing.T) {
//log error
// log error
if err2 := flag.Set("tablet_url_template", "https://{{.GetHostNameLevel 0}}.bastion.{{.Tablet.Alias.Cell}}.corp"); err2 != nil {
log.Errorf("flag.Set(\"tablet_url_template\", \"https://{{.GetHostNameLevel 0}}.bastion.{{.Tablet.Alias.Cell}}.corp\") failed : %v", err2)
}
Expand Down
Loading