From a96f1317c0c1465ea09879c075a05a5d5bdde97c Mon Sep 17 00:00:00 2001 From: Andrew Baptist Date: Tue, 31 Jan 2023 13:58:08 -0500 Subject: [PATCH] rpc: Make it explicit that we only Version from Settings Cleaning up some of the code in this package, the entire Settings object was stored in the HeartbeatService. Only the Version was required. This PR removes the unnecessary struct. Epic: none Release note: None --- pkg/rpc/context.go | 4 ++-- pkg/rpc/context_test.go | 28 ++++++++++++++-------------- pkg/rpc/heartbeat.go | 15 ++++++++------- pkg/rpc/heartbeat_test.go | 16 ++++++++-------- 4 files changed, 32 insertions(+), 31 deletions(-) diff --git a/pkg/rpc/context.go b/pkg/rpc/context.go index eb12d3c8da79..7e6ad0f910bf 100644 --- a/pkg/rpc/context.go +++ b/pkg/rpc/context.go @@ -2330,7 +2330,7 @@ func (rpcCtx *Context) runHeartbeat( } if err := errors.Wrap( - checkVersion(ctx, rpcCtx.Settings, response.ServerVersion), + checkVersion(ctx, rpcCtx.Settings.Version, response.ServerVersion), "version compatibility check failed on ping response"); err != nil { return err } @@ -2411,7 +2411,7 @@ func (rpcCtx *Context) NewHeartbeatService() *HeartbeatService { disableClusterNameVerification: rpcCtx.Config.DisableClusterNameVerification, clusterID: rpcCtx.StorageClusterID, nodeID: rpcCtx.NodeID, - settings: rpcCtx.Settings, + version: rpcCtx.Settings.Version, onHandlePing: rpcCtx.OnIncomingPing, testingAllowNamedRPCToAnonymousServer: rpcCtx.TestingAllowNamedRPCToAnonymousServer, } diff --git a/pkg/rpc/context_test.go b/pkg/rpc/context_test.go index eea2a6fe29be..2ba936882cb0 100644 --- a/pkg/rpc/context_test.go +++ b/pkg/rpc/context_test.go @@ -136,7 +136,7 @@ func TestHeartbeatCB(t *testing.T) { remoteClockMonitor: serverCtx.RemoteClocks, clusterID: serverCtx.StorageClusterID, nodeID: serverCtx.NodeID, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, }) ln, err := netutil.ListenAndServeGRPC(serverCtx.Stopper, s, util.TestAddr) @@ -873,7 +873,7 @@ func TestHeartbeatHealth(t *testing.T) { clock: clock, maxOffset: maxOffset, remoteClockMonitor: serverCtx.RemoteClocks, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, nodeID: serverCtx.NodeID, } RegisterHeartbeatServer(s, heartbeat) @@ -1122,7 +1122,7 @@ func TestHeartbeatHealthTransport(t *testing.T) { remoteClockMonitor: serverCtx.RemoteClocks, clusterID: serverCtx.StorageClusterID, nodeID: serverCtx.NodeID, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, }) mu := struct { @@ -1305,7 +1305,7 @@ func TestOffsetMeasurement(t *testing.T) { remoteClockMonitor: serverCtx.RemoteClocks, clusterID: serverCtx.StorageClusterID, nodeID: serverCtx.NodeID, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, }) ln, err := netutil.ListenAndServeGRPC(serverCtx.Stopper, s, util.TestAddr) @@ -1379,7 +1379,7 @@ func TestFailedOffsetMeasurement(t *testing.T) { remoteClockMonitor: serverCtx.RemoteClocks, ready: make(chan error), stopper: stopper, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, nodeID: serverCtx.NodeID, } RegisterHeartbeatServer(s, heartbeat) @@ -1445,7 +1445,7 @@ func TestLatencyInfoCleanupOnClosedConnection(t *testing.T) { remoteClockMonitor: serverCtx.RemoteClocks, clusterID: serverCtx.StorageClusterID, nodeID: serverCtx.NodeID, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, }) ln, err := netutil.ListenAndServeGRPC(serverCtx.Stopper, s, util.TestAddr) @@ -1584,7 +1584,7 @@ func TestRemoteOffsetUnhealthy(t *testing.T) { remoteClockMonitor: nodeCtxs[i].ctx.RemoteClocks, clusterID: nodeCtxs[i].ctx.StorageClusterID, nodeID: nodeCtxs[i].ctx.NodeID, - settings: nodeCtxs[i].ctx.Settings, + version: nodeCtxs[i].ctx.Settings.Version, }) ln, err := netutil.ListenAndServeGRPC(nodeCtxs[i].ctx.Stopper, s, util.TestAddr) if err != nil { @@ -1775,7 +1775,7 @@ func grpcRunKeepaliveTestCase(testCtx context.Context, c grpcKeepaliveTestCase) remoteClockMonitor: serverCtx.RemoteClocks, clusterID: serverCtx.StorageClusterID, nodeID: serverCtx.NodeID, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, }, interval: msgInterval, } @@ -2058,7 +2058,7 @@ func TestClusterIDMismatch(t *testing.T) { remoteClockMonitor: serverCtx.RemoteClocks, clusterID: serverCtx.StorageClusterID, nodeID: serverCtx.NodeID, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, }) ln, err := netutil.ListenAndServeGRPC(serverCtx.Stopper, s, util.TestAddr) @@ -2132,7 +2132,7 @@ func TestClusterNameMismatch(t *testing.T) { remoteClockMonitor: serverCtx.RemoteClocks, clusterID: serverCtx.StorageClusterID, nodeID: serverCtx.NodeID, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, clusterName: serverCtx.Config.ClusterName, disableClusterNameVerification: serverCtx.Config.DisableClusterNameVerification, }) @@ -2184,7 +2184,7 @@ func TestNodeIDMismatch(t *testing.T) { remoteClockMonitor: serverCtx.RemoteClocks, clusterID: serverCtx.StorageClusterID, nodeID: serverCtx.NodeID, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, }) ln, err := netutil.ListenAndServeGRPC(serverCtx.Stopper, s, util.TestAddr) @@ -2259,7 +2259,7 @@ func TestVersionCheckBidirectional(t *testing.T) { remoteClockMonitor: serverCtx.RemoteClocks, clusterID: serverCtx.StorageClusterID, nodeID: serverCtx.NodeID, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, }) ln, err := netutil.ListenAndServeGRPC(serverCtx.Stopper, s, util.TestAddr) @@ -2307,7 +2307,7 @@ func TestGRPCDialClass(t *testing.T) { remoteClockMonitor: serverCtx.RemoteClocks, clusterID: serverCtx.StorageClusterID, nodeID: serverCtx.NodeID, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, }) ln, err := netutil.ListenAndServeGRPC(serverCtx.Stopper, s, util.TestAddr) @@ -2367,7 +2367,7 @@ func TestTestingKnobs(t *testing.T) { remoteClockMonitor: serverCtx.RemoteClocks, clusterID: serverCtx.StorageClusterID, nodeID: serverCtx.NodeID, - settings: serverCtx.Settings, + version: serverCtx.Settings.Version, }) // The test will inject interceptors for both stream and unary calls and then diff --git a/pkg/rpc/heartbeat.go b/pkg/rpc/heartbeat.go index d7445634193c..cde8390a8c0c 100644 --- a/pkg/rpc/heartbeat.go +++ b/pkg/rpc/heartbeat.go @@ -18,7 +18,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/roachpb" - "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/log/severity" @@ -48,7 +47,7 @@ type HeartbeatService struct { clusterID *base.ClusterIDContainer nodeID *base.NodeIDContainer - settings *cluster.Settings + version clusterversion.Handle clusterName string disableClusterNameVerification bool @@ -80,8 +79,10 @@ func checkClusterName(clusterName string, peerName string) error { return nil } -func checkVersion(ctx context.Context, st *cluster.Settings, peerVersion roachpb.Version) error { - activeVersion := st.Version.ActiveVersionOrEmpty(ctx) +func checkVersion( + ctx context.Context, version clusterversion.Handle, peerVersion roachpb.Version, +) error { + activeVersion := version.ActiveVersionOrEmpty(ctx) if activeVersion == (clusterversion.ClusterVersion{}) { // Cluster version has not yet been determined. return nil @@ -100,7 +101,7 @@ func checkVersion(ctx context.Context, st *cluster.Settings, peerVersion roachpb minVersion := activeVersion.Version if tenantID, isTenant := roachpb.ClientTenantFromContext(ctx); isTenant && !roachpb.IsSystemTenantID(tenantID.ToUint64()) { - minVersion = st.Version.BinaryMinSupportedVersion() + minVersion = version.BinaryMinSupportedVersion() } if peerVersion.Less(minVersion) { return errors.Errorf( @@ -152,7 +153,7 @@ func (hs *HeartbeatService) Ping(ctx context.Context, args *PingRequest) (*PingR } // Check version compatibility. - if err := checkVersion(ctx, hs.settings, args.ServerVersion); err != nil { + if err := checkVersion(ctx, hs.version, args.ServerVersion); err != nil { return nil, errors.Wrap(err, "version compatibility check failed on ping request") } @@ -169,7 +170,7 @@ func (hs *HeartbeatService) Ping(ctx context.Context, args *PingRequest) (*PingR return &PingResponse{ Pong: args.Ping, ServerTime: hs.clock.Now().UnixNano(), - ServerVersion: hs.settings.Version.BinaryVersion(), + ServerVersion: hs.version.BinaryVersion(), ClusterName: hs.clusterName, DisableClusterNameVerification: hs.disableClusterNameVerification, }, nil diff --git a/pkg/rpc/heartbeat_test.go b/pkg/rpc/heartbeat_test.go index 74becdbb32c3..5f1a639656a1 100644 --- a/pkg/rpc/heartbeat_test.go +++ b/pkg/rpc/heartbeat_test.go @@ -51,7 +51,7 @@ func TestHeartbeatReply(t *testing.T) { clock: clock, remoteClockMonitor: newRemoteClockMonitor(clock, maxOffset, time.Hour, 0), clusterID: &base.ClusterIDContainer{}, - settings: st, + version: st.Version, } request := &PingRequest{ @@ -77,7 +77,7 @@ type ManualHeartbeatService struct { clock hlc.WallClock maxOffset time.Duration remoteClockMonitor *RemoteClockMonitor - settings *cluster.Settings + version clusterversion.Handle nodeID *base.NodeIDContainer // Heartbeats are processed when a value is sent here. ready chan error @@ -102,7 +102,7 @@ func (mhs *ManualHeartbeatService) Ping( clock: mhs.clock, remoteClockMonitor: mhs.remoteClockMonitor, clusterID: &base.ClusterIDContainer{}, - settings: mhs.settings, + version: mhs.version, nodeID: mhs.nodeID, } return hs.Ping(ctx, args) @@ -118,13 +118,13 @@ func TestManualHeartbeat(t *testing.T) { maxOffset: maxOffset, remoteClockMonitor: newRemoteClockMonitor(clock, maxOffset, time.Hour, 0), ready: make(chan error, 1), - settings: st, + version: st.Version, } regularHeartbeat := &HeartbeatService{ clock: clock, remoteClockMonitor: newRemoteClockMonitor(clock, maxOffset, time.Hour, 0), clusterID: &base.ClusterIDContainer{}, - settings: st, + version: st.Version, } request := &PingRequest{ @@ -176,7 +176,7 @@ func TestClusterIDCompare(t *testing.T) { clock: clock, remoteClockMonitor: newRemoteClockMonitor(clock, maxOffset, time.Hour, 0), clusterID: &base.ClusterIDContainer{}, - settings: st, + version: st.Version, } for _, td := range testData { @@ -221,7 +221,7 @@ func TestNodeIDCompare(t *testing.T) { remoteClockMonitor: newRemoteClockMonitor(clock, maxOffset, time.Hour, 0), clusterID: &base.ClusterIDContainer{}, nodeID: &base.NodeIDContainer{}, - settings: st, + version: st.Version, } for _, td := range testData { @@ -258,7 +258,7 @@ func TestTenantVersionCheck(t *testing.T) { clock: clock, remoteClockMonitor: newRemoteClockMonitor(clock, maxOffset, time.Hour, 0), clusterID: &base.ClusterIDContainer{}, - settings: st, + version: st.Version, } request := &PingRequest{