From 878f1cd907b221248c51ca9eef9df7159fb52f07 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Tue, 12 Sep 2023 19:10:03 -0700 Subject: [PATCH 1/6] sql: simplify accumulation of constraints a bit Also adjust a comment a bit. Release note: None --- pkg/sql/set_zone_config.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/pkg/sql/set_zone_config.go b/pkg/sql/set_zone_config.go index 8b3fa2c03b5..a428d3f5b93 100644 --- a/pkg/sql/set_zone_config.go +++ b/pkg/sql/set_zone_config.go @@ -966,16 +966,13 @@ func validateNoRepeatKeysInConstraints(constraints []zonepb.Constraint) error { func accumulateUniqueConstraints(zone *zonepb.ZoneConfig) []zonepb.Constraint { constraints := make([]zonepb.Constraint, 0) addToValidate := func(c zonepb.Constraint) { - var alreadyInList bool for _, val := range constraints { if c == val { - alreadyInList = true - break + // Already in the list, nothing to do. + return } } - if !alreadyInList { - constraints = append(constraints, c) - } + constraints = append(constraints, c) } for _, constraints := range zone.Constraints { for _, constraint := range constraints.Constraints { @@ -1003,7 +1000,7 @@ func accumulateUniqueConstraints(zone *zonepb.ZoneConfig) []zonepb.Constraint { // validateZoneAttrsAndLocalities is tenant aware in its validation. Secondary // tenants don't have access to the NodeStatusServer, and as such, aren't // allowed to set non-locality attributes in their constraints. Furthermore, -// their access is validated using the RegionProvider. +// their access is validated using the descs.RegionProvider. func validateZoneAttrsAndLocalities( ctx context.Context, regionProvider descs.RegionProvider, From 90c74dd412ac8ddca626da33fd6e7360058d8dd2 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Fri, 15 Sep 2023 12:57:10 -0700 Subject: [PATCH 2/6] gossip: remove no longer used system config Note that we cannot fully remove handling of `KeyDeprecatedSystemConfig` because it requires a migration. Release note: None --- pkg/gossip/gossip.go | 37 ------------------------------------- 1 file changed, 37 deletions(-) diff --git a/pkg/gossip/gossip.go b/pkg/gossip/gossip.go index 830330cf240..1ef22d08389 100644 --- a/pkg/gossip/gossip.go +++ b/pkg/gossip/gossip.go @@ -58,7 +58,6 @@ import ( "unsafe" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/config" "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" "github.com/cockroachdb/cockroach/pkg/roachpb" @@ -242,15 +241,6 @@ type Gossip struct { bootstrapInterval time.Duration cullInterval time.Duration - // The system config is treated unlike other info objects. - // It is used so often that we keep an unmarshaled version of it - // here and its own set of callbacks. - // We do not use the infostore to avoid unmarshalling under the - // main gossip lock. - systemConfig *config.SystemConfig - systemConfigMu syncutil.RWMutex - systemConfigChannels []chan<- struct{} - // addresses is a list of bootstrap host addresses for // connecting to the gossip network. addressIdx int @@ -307,8 +297,6 @@ func New( g.mu.Lock() defer g.mu.Unlock() - // Add ourselves as a SystemConfig watcher. - g.mu.is.registerCallback(KeyDeprecatedSystemConfig, g.updateSystemConfig) // Add ourselves as a node descriptor watcher. g.mu.is.registerCallback(MakePrefixPattern(KeyNodeDescPrefix), g.updateNodeAddress) g.mu.is.registerCallback(MakePrefixPattern(KeyStoreDescPrefix), g.updateStoreMap) @@ -1086,31 +1074,6 @@ func (g *Gossip) RegisterCallback(pattern string, method Callback, opts ...Callb } } -// updateSystemConfig is the raw gossip info callback. Unmarshal the -// system config, and if successful, send on each system config -// channel. -func (g *Gossip) updateSystemConfig(key string, content roachpb.Value) { - ctx := g.AnnotateCtx(context.TODO()) - if key != KeyDeprecatedSystemConfig { - log.Fatalf(ctx, "wrong key received on SystemConfig callback: %s", key) - } - cfg := config.NewSystemConfig(g.defaultZoneConfig) - if err := content.GetProto(&cfg.SystemConfigEntries); err != nil { - log.Errorf(ctx, "could not unmarshal system config on callback: %s", err) - return - } - - g.systemConfigMu.Lock() - defer g.systemConfigMu.Unlock() - g.systemConfig = cfg - for _, c := range g.systemConfigChannels { - select { - case c <- struct{}{}: - default: - } - } -} - // Incoming returns a slice of incoming gossip client connection // node IDs. func (g *Gossip) Incoming() []roachpb.NodeID { From de21a3dc4aba18dd3e295b6594a68458aeb6acb4 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Fri, 15 Sep 2023 14:41:24 -0700 Subject: [PATCH 3/6] systemconfigwatcher: move TestSystemConfigWatcher Previously, due to the requirement for having CCL license to start tenants we separated out `TestSystemConfigWatcher` into two, but that is no longer needed. Release note: None --- pkg/BUILD.bazel | 1 - pkg/ccl/serverccl/BUILD.bazel | 1 - pkg/ccl/serverccl/server_sql_test.go | 6 --- pkg/server/systemconfigwatcher/BUILD.bazel | 11 +++++- pkg/server/systemconfigwatcher/cache_test.go | 6 --- ...tcher.go => system_config_watcher_test.go} | 37 ++++++++----------- .../systemconfigwatchertest/BUILD.bazel | 28 -------------- 7 files changed, 26 insertions(+), 64 deletions(-) rename pkg/server/systemconfigwatcher/{systemconfigwatchertest/test_system_config_watcher.go => system_config_watcher_test.go} (81%) delete mode 100644 pkg/server/systemconfigwatcher/systemconfigwatchertest/BUILD.bazel diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 058bdb1cd73..eb7cadd7cd3 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -1584,7 +1584,6 @@ GO_TARGETS = [ "//pkg/server/storage_api:storage_api_test", "//pkg/server/structlogging:structlogging", "//pkg/server/structlogging:structlogging_test", - "//pkg/server/systemconfigwatcher/systemconfigwatchertest:systemconfigwatchertest", "//pkg/server/systemconfigwatcher:systemconfigwatcher", "//pkg/server/systemconfigwatcher:systemconfigwatcher_test", "//pkg/server/telemetry:telemetry", diff --git a/pkg/ccl/serverccl/BUILD.bazel b/pkg/ccl/serverccl/BUILD.bazel index a58ded298c5..91c48383af3 100644 --- a/pkg/ccl/serverccl/BUILD.bazel +++ b/pkg/ccl/serverccl/BUILD.bazel @@ -69,7 +69,6 @@ go_test( "//pkg/server", "//pkg/server/authserver", "//pkg/server/serverpb", - "//pkg/server/systemconfigwatcher/systemconfigwatchertest", "//pkg/settings/cluster", "//pkg/sql", "//pkg/sql/lexbase", diff --git a/pkg/ccl/serverccl/server_sql_test.go b/pkg/ccl/serverccl/server_sql_test.go index 30aef3fa29e..35d08c3a0d7 100644 --- a/pkg/ccl/serverccl/server_sql_test.go +++ b/pkg/ccl/serverccl/server_sql_test.go @@ -26,7 +26,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security/securitytest" "github.com/cockroachdb/cockroach/pkg/server" - "github.com/cockroachdb/cockroach/pkg/server/systemconfigwatcher/systemconfigwatchertest" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/sqlinstance/instancestorage" @@ -411,11 +410,6 @@ func TestTenantInstanceIDReclaimLoop(t *testing.T) { }) } -func TestSystemConfigWatcherCache(t *testing.T) { - defer leaktest.AfterTest(t)() - systemconfigwatchertest.TestSystemConfigWatcher(t, false /* skipSecondary */) -} - // TestStartTenantWithStaleInstance covers the following scenario: // - a sql server starts up and is assigned port 'a' // - the sql server shuts down and releases port 'a' diff --git a/pkg/server/systemconfigwatcher/BUILD.bazel b/pkg/server/systemconfigwatcher/BUILD.bazel index 6820be8f3b9..075a2c721fc 100644 --- a/pkg/server/systemconfigwatcher/BUILD.bazel +++ b/pkg/server/systemconfigwatcher/BUILD.bazel @@ -25,6 +25,7 @@ go_test( srcs = [ "cache_test.go", "main_test.go", + "system_config_watcher_test.go", ], args = ["-test.timeout=295s"], deps = [ @@ -33,18 +34,26 @@ go_test( "//pkg/config", "//pkg/config/zonepb", "//pkg/keys", + "//pkg/kv", + "//pkg/kv/kvclient/kvtenant", "//pkg/kv/kvclient/rangefeed", + "//pkg/kv/kvpb", "//pkg/roachpb", "//pkg/security/securityassets", "//pkg/security/securitytest", "//pkg/server", - "//pkg/server/systemconfigwatcher/systemconfigwatchertest", + "//pkg/sql", + "//pkg/testutils", "//pkg/testutils/serverutils", "//pkg/testutils/sqlutils", "//pkg/testutils/testcluster", "//pkg/util/leaktest", "//pkg/util/log", + "//pkg/util/protoutil", "//pkg/util/syncutil", + "@com_github_cockroachdb_errors//:errors", + "@com_github_kr_pretty//:pretty", + "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", ], ) diff --git a/pkg/server/systemconfigwatcher/cache_test.go b/pkg/server/systemconfigwatcher/cache_test.go index 997074aa007..36dbb3cb5cc 100644 --- a/pkg/server/systemconfigwatcher/cache_test.go +++ b/pkg/server/systemconfigwatcher/cache_test.go @@ -21,7 +21,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangefeed" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server/systemconfigwatcher" - "github.com/cockroachdb/cockroach/pkg/server/systemconfigwatcher/systemconfigwatchertest" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" @@ -30,11 +29,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestCache(t *testing.T) { - defer leaktest.AfterTest(t)() - systemconfigwatchertest.TestSystemConfigWatcher(t, true /* skipSecondary */) -} - func TestNewWithAdditionalProvider(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) diff --git a/pkg/server/systemconfigwatcher/systemconfigwatchertest/test_system_config_watcher.go b/pkg/server/systemconfigwatcher/system_config_watcher_test.go similarity index 81% rename from pkg/server/systemconfigwatcher/systemconfigwatchertest/test_system_config_watcher.go rename to pkg/server/systemconfigwatcher/system_config_watcher_test.go index e8b370a0a3b..0b0e056a801 100644 --- a/pkg/server/systemconfigwatcher/systemconfigwatchertest/test_system_config_watcher.go +++ b/pkg/server/systemconfigwatcher/system_config_watcher_test.go @@ -8,9 +8,7 @@ // by the Apache License, Version 2.0, included in the file // licenses/APL.txt. -// Package systemconfigwatchertest exists to exercise systemconfigwatcher -// in both ccl and non-ccl configurations. -package systemconfigwatchertest +package systemconfigwatcher_test import ( "context" @@ -39,9 +37,8 @@ import ( ) // TestSystemConfigWatcher is a test which exercises the end-to-end integration -// of the systemconfigwatcher. It exists in this subpackage so that it can be -// run to exercise secondary tenants, which are ccl-only. -func TestSystemConfigWatcher(t *testing.T, skipSecondary bool) { +// of the systemconfigwatcher +func TestSystemConfigWatcher(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) @@ -62,22 +59,20 @@ func TestSystemConfigWatcher(t *testing.T, skipSecondary bool) { t.Run("system", func(t *testing.T) { runTest(t, s, sqlDB, nil) }) - if !skipSecondary { - t.Run("secondary", func(t *testing.T) { - tenant, tenantDB := serverutils.StartTenant(t, s, base.TestTenantArgs{ - TenantID: serverutils.TestTenantID(), - }) - // We expect the secondary tenant to see the host tenant's view of a few - // keys. We need to plumb that expectation into the test. - runTest(t, tenant, tenantDB, func(t *testing.T) []roachpb.KeyValue { - return kvtenant.GossipSubscriptionSystemConfigMask.Apply( - config.SystemConfigEntries{ - Values: getSystemDescriptorAndZonesSpans(ctx, t, keys.SystemSQLCodec, kvDB), - }, - ).Values - }) + t.Run("secondary", func(t *testing.T) { + tenant, tenantDB := serverutils.StartTenant(t, s, base.TestTenantArgs{ + TenantID: serverutils.TestTenantID(), }) - } + // We expect the secondary tenant to see the host tenant's view of a few + // keys. We need to plumb that expectation into the test. + runTest(t, tenant, tenantDB, func(t *testing.T) []roachpb.KeyValue { + return kvtenant.GossipSubscriptionSystemConfigMask.Apply( + config.SystemConfigEntries{ + Values: getSystemDescriptorAndZonesSpans(ctx, t, keys.SystemSQLCodec, kvDB), + }, + ).Values + }) + }) } func runTest( diff --git a/pkg/server/systemconfigwatcher/systemconfigwatchertest/BUILD.bazel b/pkg/server/systemconfigwatcher/systemconfigwatchertest/BUILD.bazel deleted file mode 100644 index 00e648bf029..00000000000 --- a/pkg/server/systemconfigwatcher/systemconfigwatchertest/BUILD.bazel +++ /dev/null @@ -1,28 +0,0 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") - -go_library( - name = "systemconfigwatchertest", - srcs = ["test_system_config_watcher.go"], - importpath = "github.com/cockroachdb/cockroach/pkg/server/systemconfigwatcher/systemconfigwatchertest", - visibility = ["//visibility:public"], - deps = [ - "//pkg/base", - "//pkg/config", - "//pkg/keys", - "//pkg/kv", - "//pkg/kv/kvclient/kvtenant", - "//pkg/kv/kvpb", - "//pkg/roachpb", - "//pkg/sql", - "//pkg/testutils", - "//pkg/testutils/serverutils", - "//pkg/testutils/sqlutils", - "//pkg/util/leaktest", - "//pkg/util/log", - "//pkg/util/protoutil", - "@com_github_cockroachdb_errors//:errors", - "@com_github_kr_pretty//:pretty", - "@com_github_stretchr_testify//assert", - "@com_github_stretchr_testify//require", - ], -) From 3b5e4847ecac1a395e5a030a6ad24432fb37f64d Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Mon, 18 Sep 2023 20:05:12 -0400 Subject: [PATCH 4/6] gossip: remove unused default zone config reference Release note: None --- pkg/cmd/gossipsim/BUILD.bazel | 1 - pkg/cmd/gossipsim/main.go | 3 +-- pkg/gossip/BUILD.bazel | 1 - pkg/gossip/client_test.go | 7 +++---- pkg/gossip/convergence_test.go | 5 ++--- pkg/gossip/gossip.go | 17 +++-------------- pkg/gossip/gossip_test.go | 11 +++++------ pkg/gossip/simulation/BUILD.bazel | 1 - pkg/gossip/simulation/network.go | 11 ++++------- pkg/gossip/storage_test.go | 8 +++----- pkg/kv/kvclient/kvcoord/BUILD.bazel | 1 - pkg/kv/kvclient/kvcoord/dist_sender_test.go | 5 ++--- .../allocator/allocatorimpl/BUILD.bazel | 1 - .../allocator/allocatorimpl/allocator_test.go | 5 ++--- pkg/kv/kvserver/allocator/storepool/BUILD.bazel | 1 - .../allocator/storepool/test_helpers.go | 3 +-- pkg/kv/kvserver/asim/state/helpers.go | 3 +-- pkg/kv/kvserver/raft_transport_test.go | 3 +-- pkg/kv/kvserver/replica_range_lease_test.go | 6 ++---- pkg/kv/kvserver/store_test.go | 2 +- pkg/server/server.go | 1 - pkg/sql/distsql_physical_planner_test.go | 10 ++++------ pkg/sql/physicalplan/replicaoracle/BUILD.bazel | 1 - .../physicalplan/replicaoracle/oracle_test.go | 3 +-- .../localtestcluster/local_test_cluster.go | 2 +- 25 files changed, 37 insertions(+), 75 deletions(-) diff --git a/pkg/cmd/gossipsim/BUILD.bazel b/pkg/cmd/gossipsim/BUILD.bazel index 690864e688b..35bebd9427e 100644 --- a/pkg/cmd/gossipsim/BUILD.bazel +++ b/pkg/cmd/gossipsim/BUILD.bazel @@ -6,7 +6,6 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/cmd/gossipsim", visibility = ["//visibility:private"], deps = [ - "//pkg/config/zonepb", "//pkg/gossip", "//pkg/gossip/simulation", "//pkg/roachpb", diff --git a/pkg/cmd/gossipsim/main.go b/pkg/cmd/gossipsim/main.go index df4ae39de7f..49e37263779 100644 --- a/pkg/cmd/gossipsim/main.go +++ b/pkg/cmd/gossipsim/main.go @@ -62,7 +62,6 @@ import ( "strconv" "strings" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/gossip/simulation" "github.com/cockroachdb/cockroach/pkg/roachpb" @@ -302,7 +301,7 @@ func main() { stopper := stop.NewStopper() defer stopper.Stop(context.TODO()) - n := simulation.NewNetwork(stopper, nodeCount, true, zonepb.DefaultZoneConfigRef()) + n := simulation.NewNetwork(stopper, nodeCount, true) n.SimulateNetwork( func(cycle int, network *simulation.Network) bool { // Output dot graph. diff --git a/pkg/gossip/BUILD.bazel b/pkg/gossip/BUILD.bazel index 9fa95c4354b..e47e5661e85 100644 --- a/pkg/gossip/BUILD.bazel +++ b/pkg/gossip/BUILD.bazel @@ -22,7 +22,6 @@ go_library( deps = [ "//pkg/base", "//pkg/config", - "//pkg/config/zonepb", "//pkg/kv/kvpb", "//pkg/roachpb", "//pkg/rpc", diff --git a/pkg/gossip/client_test.go b/pkg/gossip/client_test.go index 9cf57a9b9be..c64f9af7f9d 100644 --- a/pkg/gossip/client_test.go +++ b/pkg/gossip/client_test.go @@ -18,7 +18,6 @@ import ( "testing" "time" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" "github.com/cockroachdb/cockroach/pkg/testutils" @@ -61,7 +60,7 @@ func startGossipAtAddr( server, err := rpc.NewServer(ctx, rpcContext) require.NoError(t, err) - g := NewTest(nodeID, stopper, registry, zonepb.DefaultZoneConfigRef()) + g := NewTest(nodeID, stopper, registry) RegisterGossipServer(server, g) ln, err := netutil.ListenAndServeGRPC(stopper, server, addr) require.NoError(t, err) @@ -121,7 +120,7 @@ func startFakeServerGossips( lserver, err := rpc.NewServer(ctx, lRPCContext) require.NoError(t, err) - local := NewTest(localNodeID, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + local := NewTest(localNodeID, stopper, metric.NewRegistry()) RegisterGossipServer(lserver, local) lln, err := netutil.ListenAndServeGRPC(stopper, lserver, util.IsolatedTestAddr) require.NoError(t, err) @@ -479,7 +478,7 @@ func TestClientRegisterWithInitNodeID(t *testing.T) { server, err := rpc.NewServer(ctx, rpcContext) require.NoError(t, err) // node ID must be non-zero - gnode := NewTest(nodeID, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + gnode := NewTest(nodeID, stopper, metric.NewRegistry()) RegisterGossipServer(server, gnode) g = append(g, gnode) diff --git a/pkg/gossip/convergence_test.go b/pkg/gossip/convergence_test.go index e664ab7c2b8..365a02a4b82 100644 --- a/pkg/gossip/convergence_test.go +++ b/pkg/gossip/convergence_test.go @@ -14,7 +14,6 @@ import ( "context" "testing" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip/simulation" "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/util/leaktest" @@ -58,7 +57,7 @@ func TestConvergence(t *testing.T) { stopper := stop.NewStopper() defer stopper.Stop(context.Background()) - network := simulation.NewNetwork(stopper, testConvergenceSize, true, zonepb.DefaultZoneConfigRef()) + network := simulation.NewNetwork(stopper, testConvergenceSize, true) const maxCycles = 100 if connectedCycle := network.RunUntilFullyConnected(); connectedCycle > maxCycles { @@ -89,7 +88,7 @@ func TestNetworkReachesEquilibrium(t *testing.T) { stopper := stop.NewStopper() defer stopper.Stop(context.Background()) - network := simulation.NewNetwork(stopper, testReachesEquilibriumSize, true, zonepb.DefaultZoneConfigRef()) + network := simulation.NewNetwork(stopper, testReachesEquilibriumSize, true) var connsRefused int64 var cyclesWithoutChange int diff --git a/pkg/gossip/gossip.go b/pkg/gossip/gossip.go index 1ef22d08389..2cdfa36c28b 100644 --- a/pkg/gossip/gossip.go +++ b/pkg/gossip/gossip.go @@ -58,7 +58,6 @@ import ( "unsafe" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" @@ -256,8 +255,6 @@ type Gossip struct { bootstrapAddrs map[util.UnresolvedAddr]roachpb.NodeID locality roachpb.Locality - - defaultZoneConfig *zonepb.ZoneConfig } // New creates an instance of a gossip node. @@ -271,7 +268,6 @@ func New( stopper *stop.Stopper, registry *metric.Registry, locality roachpb.Locality, - defaultZoneConfig *zonepb.ZoneConfig, ) *Gossip { ambient.SetEventLog("gossip", "gossip") g := &Gossip{ @@ -288,7 +284,6 @@ func New( addressExists: map[util.UnresolvedAddr]bool{}, bootstrapAddrs: map[util.UnresolvedAddr]roachpb.NodeID{}, locality: locality, - defaultZoneConfig: defaultZoneConfig, } stopper.AddCloser(stop.CloserFn(g.server.AmbientContext.FinishEventLog)) @@ -306,13 +301,8 @@ func New( // NewTest is a simplified wrapper around New that creates the // ClusterIDContainer and NodeIDContainer internally. Used for testing. -func NewTest( - nodeID roachpb.NodeID, - stopper *stop.Stopper, - registry *metric.Registry, - defaultZoneConfig *zonepb.ZoneConfig, -) *Gossip { - return NewTestWithLocality(nodeID, stopper, registry, roachpb.Locality{}, defaultZoneConfig) +func NewTest(nodeID roachpb.NodeID, stopper *stop.Stopper, registry *metric.Registry) *Gossip { + return NewTestWithLocality(nodeID, stopper, registry, roachpb.Locality{}) } // NewTestWithLocality calls NewTest with an explicit locality value. @@ -321,13 +311,12 @@ func NewTestWithLocality( stopper *stop.Stopper, registry *metric.Registry, locality roachpb.Locality, - defaultZoneConfig *zonepb.ZoneConfig, ) *Gossip { c := &base.ClusterIDContainer{} n := &base.NodeIDContainer{} var ac log.AmbientContext ac.AddLogTag("n", n) - gossip := New(ac, c, n, stopper, registry, locality, defaultZoneConfig) + gossip := New(ac, c, n, stopper, registry, locality) if nodeID != 0 { n.Set(context.TODO(), nodeID) } diff --git a/pkg/gossip/gossip_test.go b/pkg/gossip/gossip_test.go index 85cb425462f..0e8e74a73fa 100644 --- a/pkg/gossip/gossip_test.go +++ b/pkg/gossip/gossip_test.go @@ -20,7 +20,6 @@ import ( "testing" "time" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" "github.com/cockroachdb/cockroach/pkg/testutils" @@ -44,7 +43,7 @@ func TestGossipInfoStore(t *testing.T) { ctx := context.Background() stopper := stop.NewStopper() defer stopper.Stop(ctx) - g := NewTest(1, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + g := NewTest(1, stopper, metric.NewRegistry()) slice := []byte("b") if err := g.AddInfo("s", slice, time.Hour); err != nil { t.Fatal(err) @@ -64,7 +63,7 @@ func TestGossipMoveNode(t *testing.T) { ctx := context.Background() stopper := stop.NewStopper() defer stopper.Stop(ctx) - g := NewTest(1, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + g := NewTest(1, stopper, metric.NewRegistry()) var nodes []*roachpb.NodeDescriptor for i := 1; i <= 3; i++ { node := &roachpb.NodeDescriptor{ @@ -114,7 +113,7 @@ func TestGossipGetNextBootstrapAddress(t *testing.T) { util.MakeUnresolvedAddr("tcp", "localhost:9004"), } - g := NewTest(0, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + g := NewTest(0, stopper, metric.NewRegistry()) g.setAddresses(addresses) // Using specified addresses, fetch bootstrap addresses 3 times @@ -173,7 +172,7 @@ func TestGossipLocalityResolver(t *testing.T) { var node2LocalityList []roachpb.LocalityAddress node2LocalityList = append(node2LocalityList, nodeLocalityAddress2) - g := NewTestWithLocality(1, stopper, metric.NewRegistry(), gossipLocalityAdvertiseList, zonepb.DefaultZoneConfigRef()) + g := NewTestWithLocality(1, stopper, metric.NewRegistry(), gossipLocalityAdvertiseList) node1 := &roachpb.NodeDescriptor{ NodeID: 1, Address: node1PublicAddressRPC, @@ -711,7 +710,7 @@ func TestGossipJoinTwoClusters(t *testing.T) { require.NoError(t, err) // node ID must be non-zero - gnode := NewTest(roachpb.NodeID(i+1), stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + gnode := NewTest(roachpb.NodeID(i+1), stopper, metric.NewRegistry()) RegisterGossipServer(server, gnode) g = append(g, gnode) gnode.SetStallInterval(interval) diff --git a/pkg/gossip/simulation/BUILD.bazel b/pkg/gossip/simulation/BUILD.bazel index 17b8e492119..76c75e8adf3 100644 --- a/pkg/gossip/simulation/BUILD.bazel +++ b/pkg/gossip/simulation/BUILD.bazel @@ -6,7 +6,6 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/gossip/simulation", visibility = ["//visibility:public"], deps = [ - "//pkg/config/zonepb", "//pkg/gossip", "//pkg/roachpb", "//pkg/rpc", diff --git a/pkg/gossip/simulation/network.go b/pkg/gossip/simulation/network.go index a9da4b9f90e..9e7d8dac5ba 100644 --- a/pkg/gossip/simulation/network.go +++ b/pkg/gossip/simulation/network.go @@ -16,7 +16,6 @@ import ( "net" "time" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" @@ -58,9 +57,7 @@ type Network struct { } // NewNetwork creates nodeCount gossip nodes. -func NewNetwork( - stopper *stop.Stopper, nodeCount int, createAddresses bool, defaultZoneConfig *zonepb.ZoneConfig, -) *Network { +func NewNetwork(stopper *stop.Stopper, nodeCount int, createAddresses bool) *Network { ctx := context.TODO() log.Infof(ctx, "simulating gossip network with %d nodes", nodeCount) @@ -89,7 +86,7 @@ func NewNetwork( n.RPCContext.StorageClusterID.Set(context.TODO(), uuid.MakeV4()) for i := 0; i < nodeCount; i++ { - node, err := n.CreateNode(defaultZoneConfig) + node, err := n.CreateNode() if err != nil { log.Fatalf(context.TODO(), "%v", err) } @@ -103,7 +100,7 @@ func NewNetwork( } // CreateNode creates a simulation node and starts an RPC server for it. -func (n *Network) CreateNode(defaultZoneConfig *zonepb.ZoneConfig) (*Node, error) { +func (n *Network) CreateNode() (*Node, error) { ctx := context.TODO() server, err := rpc.NewServer(ctx, n.RPCContext) if err != nil { @@ -114,7 +111,7 @@ func (n *Network) CreateNode(defaultZoneConfig *zonepb.ZoneConfig) (*Node, error return nil, err } node := &Node{Server: server, Listener: ln, Registry: metric.NewRegistry()} - node.Gossip = gossip.NewTest(0, n.Stopper, node.Registry, defaultZoneConfig) + node.Gossip = gossip.NewTest(0, n.Stopper, node.Registry) gossip.RegisterGossipServer(server, node.Gossip) n.Stopper.AddCloser(stop.CloserFn(server.Stop)) _ = n.Stopper.RunAsyncTask(ctx, "node-wait-quiesce", func(context.Context) { diff --git a/pkg/gossip/storage_test.go b/pkg/gossip/storage_test.go index 9813cc02286..bd304271b63 100644 --- a/pkg/gossip/storage_test.go +++ b/pkg/gossip/storage_test.go @@ -18,7 +18,6 @@ import ( "testing" "time" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/gossip/simulation" "github.com/cockroachdb/cockroach/pkg/testutils" @@ -96,8 +95,7 @@ func TestGossipStorage(t *testing.T) { stopper := stop.NewStopper() defer stopper.Stop(context.Background()) - defaultZoneConfig := zonepb.DefaultZoneConfigRef() - network := simulation.NewNetwork(stopper, 3, true, defaultZoneConfig) + network := simulation.NewNetwork(stopper, 3, true) // Set storage for each of the nodes. addresses := make(unresolvedAddrSlice, len(network.Nodes)) @@ -154,7 +152,7 @@ func TestGossipStorage(t *testing.T) { // Create an unaffiliated gossip node with only itself as an address, // leaving it no way to reach the gossip network. - node, err := network.CreateNode(defaultZoneConfig) + node, err := network.CreateNode() if err != nil { t.Fatal(err) } @@ -211,7 +209,7 @@ func TestGossipStorageCleanup(t *testing.T) { defer stopper.Stop(context.Background()) const numNodes = 3 - network := simulation.NewNetwork(stopper, numNodes, false, zonepb.DefaultZoneConfigRef()) + network := simulation.NewNetwork(stopper, numNodes, false) const notReachableAddr = "localhost:0" const invalidAddr = "10.0.0.1000:3333333" diff --git a/pkg/kv/kvclient/kvcoord/BUILD.bazel b/pkg/kv/kvclient/kvcoord/BUILD.bazel index d90de5491fb..1b12798cd93 100644 --- a/pkg/kv/kvclient/kvcoord/BUILD.bazel +++ b/pkg/kv/kvclient/kvcoord/BUILD.bazel @@ -159,7 +159,6 @@ go_test( "//pkg/base", "//pkg/clusterversion", "//pkg/config", - "//pkg/config/zonepb", "//pkg/gossip", "//pkg/gossip/simulation", "//pkg/keys", diff --git a/pkg/kv/kvclient/kvcoord/dist_sender_test.go b/pkg/kv/kvclient/kvcoord/dist_sender_test.go index 2e9a3d08db8..cd41e176982 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender_test.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender_test.go @@ -25,7 +25,6 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/gossip/simulation" "github.com/cockroachdb/cockroach/pkg/keys" @@ -220,7 +219,7 @@ func (l *simpleTransportAdapter) Release() {} func makeGossip(t *testing.T, stopper *stop.Stopper, rpcContext *rpc.Context) *gossip.Gossip { const nodeID = 1 - g := gossip.NewTest(nodeID, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + g := gossip.NewTest(nodeID, stopper, metric.NewRegistry()) if err := g.SetNodeDescriptor(newNodeDesc(nodeID)); err != nil { t.Fatal(err) } @@ -2021,7 +2020,7 @@ func TestGetFirstRangeDescriptor(t *testing.T) { stopper := stop.NewStopper(stop.WithTracer(tr)) defer stopper.Stop(context.Background()) - n := simulation.NewNetwork(stopper, 3, true, zonepb.DefaultZoneConfigRef()) + n := simulation.NewNetwork(stopper, 3, true) for _, node := range n.Nodes { // TODO(spencer): remove the use of gossip/simulation here. node.Gossip.EnableSimulationCycler(false) diff --git a/pkg/kv/kvserver/allocator/allocatorimpl/BUILD.bazel b/pkg/kv/kvserver/allocator/allocatorimpl/BUILD.bazel index 405f9458d22..71c9e3213dc 100644 --- a/pkg/kv/kvserver/allocator/allocatorimpl/BUILD.bazel +++ b/pkg/kv/kvserver/allocator/allocatorimpl/BUILD.bazel @@ -44,7 +44,6 @@ go_test( args = ["-test.timeout=295s"], embed = [":allocatorimpl"], deps = [ - "//pkg/config/zonepb", "//pkg/gossip", "//pkg/keys", "//pkg/kv/kvpb", diff --git a/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go b/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go index 33e39bfcd89..de35ebcc43a 100644 --- a/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go +++ b/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go @@ -22,7 +22,6 @@ import ( "testing" "time" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" @@ -8458,7 +8457,7 @@ func TestAllocatorFullDisks(t *testing.T) { tr := tracing.NewTracer() clock := hlc.NewClockForTesting(nil) - g := gossip.NewTest(1, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + g := gossip.NewTest(1, stopper, metric.NewRegistry()) liveness.TimeUntilNodeDead.Override(ctx, &st.SV, liveness.TestTimeUntilNodeDeadOff) @@ -8916,7 +8915,7 @@ func exampleRebalancing( // Model a set of stores in a cluster, // adding / rebalancing ranges of random sizes. - g := gossip.NewTest(1, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + g := gossip.NewTest(1, stopper, metric.NewRegistry()) liveness.TimeUntilNodeDead.Override(ctx, &st.SV, liveness.TestTimeUntilNodeDeadOff) diff --git a/pkg/kv/kvserver/allocator/storepool/BUILD.bazel b/pkg/kv/kvserver/allocator/storepool/BUILD.bazel index 8a6792c32e5..3affd986de9 100644 --- a/pkg/kv/kvserver/allocator/storepool/BUILD.bazel +++ b/pkg/kv/kvserver/allocator/storepool/BUILD.bazel @@ -10,7 +10,6 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/storepool", visibility = ["//visibility:public"], deps = [ - "//pkg/config/zonepb", "//pkg/gossip", "//pkg/kv/kvserver/allocator", "//pkg/kv/kvserver/allocator/load", diff --git a/pkg/kv/kvserver/allocator/storepool/test_helpers.go b/pkg/kv/kvserver/allocator/storepool/test_helpers.go index 9725a6a8928..33fe34c2924 100644 --- a/pkg/kv/kvserver/allocator/storepool/test_helpers.go +++ b/pkg/kv/kvserver/allocator/storepool/test_helpers.go @@ -14,7 +14,6 @@ import ( "context" "time" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb" @@ -79,7 +78,7 @@ func CreateTestStorePool( mc := timeutil.NewManualTime(time.Date(2020, 0, 0, 0, 0, 0, 0, time.UTC)) clock := hlc.NewClockForTesting(mc) ambientCtx := log.MakeTestingAmbientContext(stopper.Tracer()) - g := gossip.NewTest(1, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + g := gossip.NewTest(1, stopper, metric.NewRegistry()) mnl := NewMockNodeLiveness(defaultNodeStatus) liveness.TimeUntilNodeDead.Override(ctx, &st.SV, timeUntilNodeDeadValue) diff --git a/pkg/kv/kvserver/asim/state/helpers.go b/pkg/kv/kvserver/asim/state/helpers.go index f5b7513e49a..ad74c458068 100644 --- a/pkg/kv/kvserver/asim/state/helpers.go +++ b/pkg/kv/kvserver/asim/state/helpers.go @@ -15,7 +15,6 @@ import ( "math/rand" "time" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/storepool" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/load" @@ -74,7 +73,7 @@ func NewStorePool( ambientCtx := log.MakeTestingAmbientContext(stopper.Tracer()) // Never gossip, pass in nil values. - g := gossip.NewTest(1, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + g := gossip.NewTest(1, stopper, metric.NewRegistry()) sp := storepool.NewStorePool( ambientCtx, st, diff --git a/pkg/kv/kvserver/raft_transport_test.go b/pkg/kv/kvserver/raft_transport_test.go index 0bc1d65a6b9..8819bec30e6 100644 --- a/pkg/kv/kvserver/raft_transport_test.go +++ b/pkg/kv/kvserver/raft_transport_test.go @@ -18,7 +18,6 @@ import ( "testing" "time" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" "github.com/cockroachdb/cockroach/pkg/kv/kvserver" @@ -138,7 +137,7 @@ func newRaftTransportTestContext(t testing.TB, st *cluster.Settings) *raftTransp // we can't enforce some of the RPC check validation. rttc.nodeRPCContext.TestingAllowNamedRPCToAnonymousServer = true - rttc.gossip = gossip.NewTest(1, rttc.stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + rttc.gossip = gossip.NewTest(1, rttc.stopper, metric.NewRegistry()) return rttc } diff --git a/pkg/kv/kvserver/replica_range_lease_test.go b/pkg/kv/kvserver/replica_range_lease_test.go index 10b50e977fb..f8877391a6c 100644 --- a/pkg/kv/kvserver/replica_range_lease_test.go +++ b/pkg/kv/kvserver/replica_range_lease_test.go @@ -15,7 +15,6 @@ import ( "testing" "time" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness" @@ -125,9 +124,8 @@ func TestReplicaLeaseStatus(t *testing.T) { } { t.Run("", func(t *testing.T) { l := liveness.NewNodeLiveness(liveness.NodeLivenessOptions{ - Clock: clock, - Gossip: gossip.NewTest(roachpb.NodeID(1), stopper, metric.NewRegistry(), - zonepb.DefaultZoneConfigRef()), + Clock: clock, + Gossip: gossip.NewTest(roachpb.NodeID(1), stopper, metric.NewRegistry()), Settings: cluster.MakeTestingClusterSettings(), }) r := Replica{store: &Store{ diff --git a/pkg/kv/kvserver/store_test.go b/pkg/kv/kvserver/store_test.go index 9d3db54f95f..7e93f81973a 100644 --- a/pkg/kv/kvserver/store_test.go +++ b/pkg/kv/kvserver/store_test.go @@ -196,7 +196,7 @@ func createTestStoreWithoutStart( // TestChooseLeaseToTransfer and TestNoLeaseTransferToBehindReplicas. This is // generally considered bad and should eventually be refactored away. if cfg.Gossip == nil { - cfg.Gossip = gossip.NewTest(1, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + cfg.Gossip = gossip.NewTest(1, stopper, metric.NewRegistry()) } if cfg.StorePool == nil { cfg.StorePool = NewTestStorePool(*cfg) diff --git a/pkg/server/server.go b/pkg/server/server.go index c27416b1b03..12a89c8a781 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -312,7 +312,6 @@ func NewServer(cfg Config, stopper *stop.Stopper) (serverctl.ServerStartupInterf stopper, nodeRegistry, cfg.Locality, - &cfg.DefaultZoneConfig, ) tenantCapabilitiesTestingKnobs, _ := cfg.TestingKnobs.TenantCapabilitiesTestingKnobs.(*tenantcapabilities.TestingKnobs) diff --git a/pkg/sql/distsql_physical_planner_test.go b/pkg/sql/distsql_physical_planner_test.go index 6dd5abf115b..ac79701093c 100644 --- a/pkg/sql/distsql_physical_planner_test.go +++ b/pkg/sql/distsql_physical_planner_test.go @@ -21,7 +21,6 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" @@ -839,7 +838,7 @@ func TestPartitionSpans(t *testing.T) { // DistSQLPlanner will not plan flows on them. testStopper := stop.NewStopper() defer testStopper.Stop(context.Background()) - mockGossip := gossip.NewTest(roachpb.NodeID(1), testStopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + mockGossip := gossip.NewTest(roachpb.NodeID(1), testStopper, metric.NewRegistry()) var nodeDescs []*roachpb.NodeDescriptor mockInstances := make(mockAddressResolver) for i := 1; i <= 10; i++ { @@ -1049,7 +1048,7 @@ func TestPartitionSpansSkipsIncompatibleNodes(t *testing.T) { // reflect tc.nodesNotAdvertisingDistSQLVersion. testStopper := stop.NewStopper() defer testStopper.Stop(context.Background()) - mockGossip := gossip.NewTest(roachpb.NodeID(1), testStopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + mockGossip := gossip.NewTest(roachpb.NodeID(1), testStopper, metric.NewRegistry()) var nodeDescs []*roachpb.NodeDescriptor for i := 1; i <= 2; i++ { sqlInstanceID := base.SQLInstanceID(i) @@ -1143,7 +1142,7 @@ func TestPartitionSpansSkipsNodesNotInGossip(t *testing.T) { stopper := stop.NewStopper() defer stopper.Stop(context.Background()) - mockGossip := gossip.NewTest(roachpb.NodeID(1), stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + mockGossip := gossip.NewTest(roachpb.NodeID(1), stopper, metric.NewRegistry()) var nodeDescs []*roachpb.NodeDescriptor for i := 1; i <= 2; i++ { sqlInstanceID := base.SQLInstanceID(i) @@ -1238,8 +1237,7 @@ func TestCheckNodeHealth(t *testing.T) { const sqlInstanceID = base.SQLInstanceID(5) - mockGossip := gossip.NewTest(roachpb.NodeID(sqlInstanceID), - stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + mockGossip := gossip.NewTest(roachpb.NodeID(sqlInstanceID), stopper, metric.NewRegistry()) desc := &roachpb.NodeDescriptor{ NodeID: roachpb.NodeID(sqlInstanceID), diff --git a/pkg/sql/physicalplan/replicaoracle/BUILD.bazel b/pkg/sql/physicalplan/replicaoracle/BUILD.bazel index 89c4dcf7b8c..90e4db5607a 100644 --- a/pkg/sql/physicalplan/replicaoracle/BUILD.bazel +++ b/pkg/sql/physicalplan/replicaoracle/BUILD.bazel @@ -25,7 +25,6 @@ go_test( args = ["-test.timeout=55s"], embed = [":replicaoracle"], deps = [ - "//pkg/config/zonepb", "//pkg/gossip", "//pkg/roachpb", "//pkg/testutils", diff --git a/pkg/sql/physicalplan/replicaoracle/oracle_test.go b/pkg/sql/physicalplan/replicaoracle/oracle_test.go index 0fe7b7ad79d..a461669fc37 100644 --- a/pkg/sql/physicalplan/replicaoracle/oracle_test.go +++ b/pkg/sql/physicalplan/replicaoracle/oracle_test.go @@ -17,7 +17,6 @@ import ( "testing" "time" - "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/testutils" @@ -88,7 +87,7 @@ func makeGossip(t *testing.T, stopper *stop.Stopper, nodeIDs []int) (*gossip.Gos clock := hlc.NewClockForTesting(nil) const nodeID = 1 - g := gossip.NewTest(nodeID, stopper, metric.NewRegistry(), zonepb.DefaultZoneConfigRef()) + g := gossip.NewTest(nodeID, stopper, metric.NewRegistry()) if err := g.SetNodeDescriptor(newNodeDesc(nodeID)); err != nil { t.Fatal(err) } diff --git a/pkg/testutils/localtestcluster/local_test_cluster.go b/pkg/testutils/localtestcluster/local_test_cluster.go index 706126aaa48..01dd7d1c673 100644 --- a/pkg/testutils/localtestcluster/local_test_cluster.go +++ b/pkg/testutils/localtestcluster/local_test_cluster.go @@ -154,7 +154,7 @@ func (ltc *LocalTestCluster) Start(t testing.TB, initFactory InitFactoryFn) { cfg.RPCContext.NodeID.Set(ctx, nodeID) clusterID := cfg.RPCContext.StorageClusterID - ltc.Gossip = gossip.New(ambient, clusterID, nc, ltc.stopper, metric.NewRegistry(), roachpb.Locality{}, zonepb.DefaultZoneConfigRef()) + ltc.Gossip = gossip.New(ambient, clusterID, nc, ltc.stopper, metric.NewRegistry(), roachpb.Locality{}) var err error ltc.Eng, err = storage.Open( ctx, From 6b81d97c1d23d8a0ebd7ab679648dbf49443ed74 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Mon, 18 Sep 2023 21:07:04 -0400 Subject: [PATCH 5/6] systemconfigwatcher: remove no longer used timestamp in Cache Previously this was used in tests but they have been deleted at some point. Release note: None --- pkg/server/systemconfigwatcher/cache.go | 28 +++++++------------------ 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/pkg/server/systemconfigwatcher/cache.go b/pkg/server/systemconfigwatcher/cache.go index 595ab411328..245777e6bfa 100644 --- a/pkg/server/systemconfigwatcher/cache.go +++ b/pkg/server/systemconfigwatcher/cache.go @@ -37,8 +37,7 @@ type Cache struct { mu struct { syncutil.RWMutex - cfg *config.SystemConfig - timestamp hlc.Timestamp + cfg *config.SystemConfig registry notificationRegistry @@ -173,15 +172,6 @@ func (c *Cache) RegisterSystemConfigChannel() (_ <-chan struct{}, unregister fun } } -// LastUpdated returns the timestamp corresponding to the current state of -// the cache. Any subsequent call to GetSystemConfig will see a state that -// corresponds to a snapshot as least as new as this timestamp. -func (c *Cache) LastUpdated() hlc.Timestamp { - c.mu.RLock() - defer c.mu.RUnlock() - return c.mu.timestamp -} - func (c *Cache) setAdditionalKeys(kvs []roachpb.KeyValue) { c.mu.Lock() defer c.mu.Unlock() @@ -240,29 +230,25 @@ func (c *Cache) handleUpdate(_ context.Context, update rangefeedcache.Update) { case rangefeedcache.CompleteUpdate: updatedData = rangefeedbuffer.MergeKVs(c.mu.additionalKVs, updateKVs) case rangefeedcache.IncrementalUpdate: + if len(updateKVs) == 0 { + // Simply return since there is nothing interesting. + return + } // Note that handleUpdate is called synchronously, so we can use the // old snapshot as the basis for the new snapshot without any risk of // missing anything. prev := c.mu.cfg - - // If there is nothing interesting, just update the timestamp and - // return without notifying anybody. - if len(updateKVs) == 0 { - c.setUpdatedConfigLocked(prev, update.Timestamp) - return - } updatedData = rangefeedbuffer.MergeKVs(prev.Values, updateKVs) } updatedCfg := config.NewSystemConfig(c.defaultZoneConfig) updatedCfg.Values = updatedData - c.setUpdatedConfigLocked(updatedCfg, update.Timestamp) + c.setUpdatedConfigLocked(updatedCfg) } -func (c *Cache) setUpdatedConfigLocked(updated *config.SystemConfig, ts hlc.Timestamp) { +func (c *Cache) setUpdatedConfigLocked(updated *config.SystemConfig) { changed := c.mu.cfg != updated c.mu.cfg = updated - c.mu.timestamp = ts if changed { c.mu.registry.notify() } From 9379c63d5bfd2e7c1c5e10a469b8a76a48bd5b0f Mon Sep 17 00:00:00 2001 From: sumeerbhola Date: Wed, 20 Sep 2023 14:35:26 -0400 Subject: [PATCH 6/6] kvflowcontroller: add trace statement at end of wait Epic: none Release note: None --- .../kvflowcontroller/kvflowcontroller.go | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/pkg/kv/kvserver/kvflowcontrol/kvflowcontroller/kvflowcontroller.go b/pkg/kv/kvserver/kvflowcontrol/kvflowcontroller/kvflowcontroller.go index a4980e4aff8..ea539660de4 100644 --- a/pkg/kv/kvserver/kvflowcontrol/kvflowcontroller/kvflowcontroller.go +++ b/pkg/kv/kvserver/kvflowcontrol/kvflowcontroller/kvflowcontroller.go @@ -144,6 +144,7 @@ func (c *Controller) Admit( class := admissionpb.WorkClassFromPri(pri) c.metrics.onWaiting(class) + // The value of logged transitions to true if there is any waiting. logged := false tstart := c.clock.PhysicalTime() for { @@ -155,10 +156,15 @@ func (c *Controller) Admit( // applying flow control to their specific work class. bypass := c.mode() == kvflowcontrol.ApplyToElastic && class == admissionpb.RegularWorkClass if tokens > 0 || bypass { - if log.V(2) { - log.Infof(ctx, "admitted request (pri=%s stream=%s tokens=%s wait-duration=%s mode=%s)", - pri, connection.Stream(), tokens, c.clock.PhysicalTime().Sub(tstart), c.mode()) + waitDuration := c.clock.PhysicalTime().Sub(tstart) + const formatStr = "admitted request (pri=%s stream=%s tokens=%s wait-duration=%s mode=%s)" + if logged { + // Always trace if there is any waiting. + log.VEventf(ctx, 2, formatStr, pri, connection.Stream(), tokens, waitDuration, c.mode()) + } else if log.V(2) { + log.Infof(ctx, formatStr, pri, connection.Stream(), tokens, waitDuration, c.mode()) } + // Else, common case, no waiting and logging verbosity is not high. // TODO(irfansharif): Right now we continue forwarding admission // grants to request while the available tokens > 0, which can lead @@ -179,7 +185,7 @@ func (c *Controller) Admit( // that count is greater than some small multiple of GOMAXPROCS. b.signal() // signal a waiter, if any - c.metrics.onAdmitted(class, c.clock.PhysicalTime().Sub(tstart)) + c.metrics.onAdmitted(class, waitDuration) if bypass { return false, nil } @@ -195,12 +201,23 @@ func (c *Controller) Admit( select { case <-b.wait(): // wait for a signal case <-connection.Disconnected(): - c.metrics.onBypassed(class, c.clock.PhysicalTime().Sub(tstart)) + waitDuration := c.clock.PhysicalTime().Sub(tstart) + log.VEventf(ctx, 2, + "bypassed as stream disconnected (pri=%s stream=%s tokens=%s wait-duration=%s mode=%s)", + pri, connection.Stream(), tokens, waitDuration, c.mode()) + c.metrics.onBypassed(class, waitDuration) return true, nil case <-ctx.Done(): if ctx.Err() != nil { - c.metrics.onErrored(class, c.clock.PhysicalTime().Sub(tstart)) + waitDuration := c.clock.PhysicalTime().Sub(tstart) + log.VEventf(ctx, 2, + "canceled after waiting (pri=%s stream=%s tokens=%s wait-duration=%s mode=%s)", + pri, connection.Stream(), tokens, waitDuration, c.mode()) + c.metrics.onErrored(class, waitDuration) } + // Else ... + // TODO(sumeer): when would error be non-nil? + return false, ctx.Err() } }