[healthcheck] Remove deprecated fields from TabletHealth json marshal…

…ling (#12429) * Remove deprecated fields from TabletHealth json marshalling Plus, as a bonus, resolve a few TODO comments in the tests Signed-off-by: Andrew Mason <[email protected]> * Update vtctld test now that fields are removed Signed-off-by: Andrew Mason <[email protected]> * fix merge conflict Signed-off-by: Andrew Mason <[email protected]> --------- Signed-off-by: Andrew Mason <[email protected]>
vitessio · Mar 1, 2023 · 467972a · 467972a
1 parent c2fec5b
commit 467972a
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 44 deletions.
diff --git a/doc/releasenotes/17_0_0_summary.md b/doc/releasenotes/17_0_0_summary.md
@@ -171,3 +171,4 @@ Some notes to help understand these metrics:
 * The deprecated `automation` and `automationservice` protobuf definitions and associated client and server packages have been removed.
 * Auto-population of DDL revert actions and tables at execution-time has been removed. This is now handled entirely at enqueue-time.
 * Backwards-compatibility for failed migrations without a `completed_timestamp` has been removed (see https://github.com/vitessio/vitess/issues/8499).
+* The deprecated `Key`, `Name`, `Up`, and `TabletExternallyReparentedTimestamp` fields were removed from the JSON representation of `TabletHealth` structures.
diff --git a/go/vt/discovery/healthcheck_test.go b/go/vt/discovery/healthcheck_test.go
@@ -196,8 +196,9 @@ func TestHealthCheck(t *testing.T) {
 	}
 	input <- shr
 	result = <-resultChan
-	// 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)
+	// Ignore LastError because we're going to check it separately.
+	utils.MustMatchFn(".LastError", ".Conn")(t, want, result, "Wrong TabletHealth data")
+	assert.Error(t, result.LastError, "vttablet error: some error")
 	testChecksum(t, 1027934207, hc.stateChecksum()) // unchanged
 
 	// remove tablet
@@ -257,8 +258,9 @@ 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
-	assert.True(t, want.DeepEqual(result), "Wrong TabletHealth data\n Expected: %v\n Actual:   %v", want, result)
+	// Ignore LastError because we're going to check it separately.
+	utils.MustMatchFn(".LastError", ".Conn")(t, want, result, "Wrong TabletHealth data")
+	assert.Error(t, result.LastError, "some stream error")
 	// tablet should be removed from healthy list
 	a := hc.GetHealthyTabletStats(&querypb.Target{Keyspace: "k", Shard: "s", TabletType: topodatapb.TabletType_REPLICA})
 	assert.Empty(t, a, "wrong result, expected empty list")
@@ -317,8 +319,9 @@ 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
-	assert.True(t, want.DeepEqual(result), "Wrong TabletHealth data\n Expected: %v\n Actual:   %v", want, result)
+	// Ignore LastError because we're going to check it separately.
+	utils.MustMatchFn(".LastError", ".Conn")(t, want, result, "Wrong TabletHealth data")
+	assert.Error(t, result.LastError, "some stream error")
 	// tablet should be removed from healthy list
 	a := hc.GetHealthyTabletStats(&querypb.Target{Keyspace: "k", Shard: "s", TabletType: topodatapb.TabletType_PRIMARY})
 	assert.Empty(t, a, "wrong result, expected empty list")

diff --git a/go/vt/discovery/tablet_health.go b/go/vt/discovery/tablet_health.go
@@ -21,8 +21,6 @@ import (
 	"encoding/json"
 	"strings"
 
-	"vitess.io/vitess/go/vt/topo/topoproto"
-
 	"vitess.io/vitess/go/vt/vttablet/queryservice"
 
 	"google.golang.org/protobuf/proto"
@@ -46,38 +44,19 @@ type TabletHealth struct {
 
 func (th *TabletHealth) MarshalJSON() ([]byte, error) {
 	return json.Marshal(&struct {
-		Key                                 string
-		Tablet                              *topodata.Tablet
-		Name                                string
-		Target                              *query.Target
-		Up                                  bool
-		Serving                             bool
-		PrimaryTermStartTime                int64
-		TabletExternallyReparentedTimestamp int64
-		Stats                               *query.RealtimeStats
-		LastError                           error
+		Tablet               *topodata.Tablet
+		Target               *query.Target
+		Serving              bool
+		PrimaryTermStartTime int64
+		Stats                *query.RealtimeStats
+		LastError            error
 	}{
-		// The Key and Name fields are fields that used to live in the legacy tablet health
-		// TODO: remove Key and Name in v15
-		Key:    TabletToMapKey(th.Tablet),
-		Tablet: th.Tablet,
-		Name:   topoproto.TabletAliasString(th.Tablet.Alias),
-		Target: th.Target,
-
-		// Setting Up to true to ensure backward compatibility
-		// TODO: remove Up in v15
-		Up: true,
-
-		Serving: th.Serving,
-
-		// We copy the PrimaryTermStartTime value onto TabletExternallyReparentedTimestamp to
-		// ensure backward compatibility.
-		// TODO: remove Up in v15
-		PrimaryTermStartTime:                th.PrimaryTermStartTime,
-		TabletExternallyReparentedTimestamp: th.PrimaryTermStartTime,
-
-		Stats:     th.Stats,
-		LastError: th.LastError,
+		Tablet:               th.Tablet,
+		Target:               th.Target,
+		Serving:              th.Serving,
+		PrimaryTermStartTime: th.PrimaryTermStartTime,
+		Stats:                th.Stats,
+		LastError:            th.LastError,
 	})
 }
 

diff --git a/go/vt/vtctld/api_test.go b/go/vt/vtctld/api_test.go
@@ -57,9 +57,8 @@ func tabletStats(keyspace, cell, shard string, tabletType topodatapb.TabletType,
 		ReplicationLagSeconds: uid,
 	}
 	stats := &discovery.TabletHealth{
-		Tablet: tablet,
-		Target: target,
-		// Up:        true,
+		Tablet:    tablet,
+		Target:    target,
 		Serving:   true,
 		Stats:     realtimeStats,
 		LastError: nil,
@@ -419,8 +418,8 @@ func TestAPI(t *testing.T) {
 		{"GET", "tablet_statuses/?keyspace=ks1&cell=cell1&type=hello&metric=lag", "", "can't get tablet_statuses: invalid tablet type: unknown TabletType hello", http.StatusInternalServerError},
 
 		// Tablet Health
-		{"GET", "tablet_health/cell1/100", "", `{ "Key": ",grpc:101,vt:100", "Tablet": { "alias": { "cell": "cell1", "uid": 100 },"port_map": { "grpc": 101, "vt": 100 }, "keyspace": "ks1", "shard": "-80", "type": 2},
-		  "Name": "cell1-0000000100", "Target": { "keyspace": "ks1", "shard": "-80", "tablet_type": 2 }, "Up": true, "Serving": true, "PrimaryTermStartTime": 0, "TabletExternallyReparentedTimestamp": 0,
+		{"GET", "tablet_health/cell1/100", "", `{ "Tablet": { "alias": { "cell": "cell1", "uid": 100 },"port_map": { "grpc": 101, "vt": 100 }, "keyspace": "ks1", "shard": "-80", "type": 2},
+		  "Target": { "keyspace": "ks1", "shard": "-80", "tablet_type": 2 }, "Serving": true, "PrimaryTermStartTime": 0,
 		  "Stats": { "replication_lag_seconds": 100 }, "LastError": null }`, http.StatusOK},
 		{"GET", "tablet_health/cell1", "", "can't get tablet_health: invalid tablet_health path: \"cell1\"  expected path: /tablet_health/<cell>/<uid>", http.StatusInternalServerError},
 		{"GET", "tablet_health/cell1/gh", "", "can't get tablet_health: incorrect uid", http.StatusInternalServerError},