Skip to content

Commit

Permalink
Merge #37434
Browse files Browse the repository at this point in the history 
37434: server: remove global defaultZoneConfig r=ajwerner,nvanbenschoten a=jeffrey-xiao

Previously the default zone config was stored as a global and code in various layers would use `config.DefaultZoneConfig()` directly. It was difficult for parallel tests to modify the global and restore it
correctly.

The default zone config can be overridden in tests by setting the `DefaultZoneConfigOverride` testing knob.

Fixes #37054

Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>
  • Loading branch information
@jeffrey-xiao
craig[bot] and jeffrey-xiao committed May 14, 2019
2 parents ba58a7e + 250aa3e commit dd997fd
Show file tree
Hide file tree
Showing 68 changed files with 382 additions and 373 deletions.
6 changes: 4 additions & 2 deletions pkg/ccl/partitionccl/partition_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/config"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/server"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/parser"
Expand Down Expand Up @@ -1108,7 +1109,6 @@ func setupPartitioningTestCluster(
) (*gosql.DB, *sqlutils.SQLRunner, func()) {
cfg := config.DefaultZoneConfig()
cfg.NumReplicas = proto.Int32(1)
resetZoneConfig := config.TestingSetDefaultZoneConfig(cfg)

tsArgs := func(attr string) base.TestServerArgs {
return base.TestServerArgs{
Expand All @@ -1118,6 +1118,9 @@ func setupPartitioningTestCluster(
// below, it's possible that one of the system ranges trigger a split.
DisableLoadBasedSplitting: true,
},
Server: &server.TestingKnobs{
DefaultZoneConfigOverride: &cfg,
},
},
ScanInterval: 100 * time.Millisecond,
StoreSpecs: []base.StoreSpec{
Expand All @@ -1142,7 +1145,6 @@ func setupPartitioningTestCluster(

return tc.Conns[0], sqlDB, func() {
tc.Stopper().Stop(context.Background())
resetZoneConfig()
}
}

Expand Down
7 changes: 4 additions & 3 deletions pkg/ccl/partitionccl/zone_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/config"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/server"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
"github.com/cockroachdb/cockroach/pkg/testutils"
Expand All @@ -43,9 +44,9 @@ func TestValidIndexPartitionSetShowZones(t *testing.T) {
PARTITION p1 VALUES IN (DEFAULT)
)`)

yamlDefault := fmt.Sprintf("gc: {ttlseconds: %d}", config.DefaultZoneConfig().GC.TTLSeconds)
yamlDefault := fmt.Sprintf("gc: {ttlseconds: %d}", s.(*server.TestServer).Cfg.DefaultZoneConfig.GC.TTLSeconds)
yamlOverride := "gc: {ttlseconds: 42}"
zoneOverride := config.DefaultZoneConfig()
zoneOverride := s.(*server.TestServer).Cfg.DefaultZoneConfig
zoneOverride.GC = &config.GCPolicy{TTLSeconds: 42}
partialZoneOverride := *config.NewZoneConfig()
partialZoneOverride.GC = &config.GCPolicy{TTLSeconds: 42}
Expand All @@ -56,7 +57,7 @@ func TestValidIndexPartitionSetShowZones(t *testing.T) {
defaultRow := sqlutils.ZoneRow{
ID: keys.RootNamespaceID,
CLISpecifier: ".default",
Config: config.DefaultZoneConfig(),
Config: s.(*server.TestServer).Cfg.DefaultZoneConfig,
}
defaultOverrideRow := sqlutils.ZoneRow{
ID: keys.RootNamespaceID,
Expand Down
14 changes: 7 additions & 7 deletions pkg/cli/demo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,21 +100,21 @@ func setupTransientServer(
cfg := config.DefaultZoneConfig()
cfg.NumReplicas = proto.Int32(1)

// TODO(benesch): should this use TestingSetDefaultZone config instead?
restoreCfg := config.TestingSetDefaultSystemZoneConfig(cfg)
prevCleanup := cleanup
cleanup = func() { prevCleanup(); restoreCfg() }

// Create the transient server.
args := base.TestServerArgs{
Insecure: true,
Knobs: base.TestingKnobs{
Server: &server.TestingKnobs{
DefaultZoneConfigOverride: &cfg,
},
},
}
server := server.TestServerFactory.New(args).(*server.TestServer)
if err := server.Start(args); err != nil {
return connURL, adminURL, cleanup, err
}
prevCleanup2 := cleanup
cleanup = func() { prevCleanup2(); server.Stopper().Stop(ctx) }
prevCleanup := cleanup
cleanup = func() { prevCleanup(); server.Stopper().Stop(ctx) }

// Prepare the URL for use by the SQL shell.
options := url.Values{}
Expand Down
3 changes: 2 additions & 1 deletion pkg/cmd/gossipsim/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ import (
"strconv"
"strings"

"github.com/cockroachdb/cockroach/pkg/config"
"github.com/cockroachdb/cockroach/pkg/gossip"
"github.com/cockroachdb/cockroach/pkg/gossip/simulation"
"github.com/cockroachdb/cockroach/pkg/roachpb"
Expand Down Expand Up @@ -306,7 +307,7 @@ func main() {
stopper := stop.NewStopper()
defer stopper.Stop(context.TODO())

n := simulation.NewNetwork(stopper, nodeCount, true)
n := simulation.NewNetwork(stopper, nodeCount, true, config.DefaultZoneConfigRef())
n.SimulateNetwork(
func(cycle int, network *simulation.Network) bool {
// Output dot graph.
Expand Down
8 changes: 5 additions & 3 deletions pkg/config/system.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,16 +71,18 @@ type zoneEntry struct {
// that should not be considered for splits.
type SystemConfig struct {
SystemConfigEntries
mu struct {
DefaultZoneConfig *ZoneConfig
mu struct {
syncutil.RWMutex
zoneCache map[uint32]zoneEntry
shouldSplitCache map[uint32]bool
}
}

// NewSystemConfig returns an initialized instance of SystemConfig.
func NewSystemConfig() *SystemConfig {
func NewSystemConfig(defaultZoneConfig *ZoneConfig) *SystemConfig {
sc := &SystemConfig{}
sc.DefaultZoneConfig = defaultZoneConfig
sc.mu.zoneCache = map[uint32]zoneEntry{}
sc.mu.shouldSplitCache = map[uint32]bool{}
return sc
Expand Down Expand Up @@ -355,7 +357,7 @@ func (s *SystemConfig) getZoneConfigForKey(id uint32, keySuffix []byte) (*ZoneCo
}
return entry.zone, nil
}
return DefaultZoneConfigRef(), nil
return s.DefaultZoneConfig, nil
}

var staticSplits = []roachpb.RKey{
Expand Down
18 changes: 9 additions & 9 deletions pkg/config/system_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func TestGet(t *testing.T) {
{someKeys, "d", &cVal},
}

cfg := config.NewSystemConfig()
cfg := config.NewSystemConfig(config.DefaultZoneConfigRef())
for tcNum, tc := range testCases {
cfg.Values = tc.values
if val := cfg.GetValue([]byte(tc.key)); !proto.Equal(val, tc.value) {
Expand Down Expand Up @@ -164,7 +164,7 @@ func TestGetLargestID(t *testing.T) {

// Real SQL layout.
func() testCase {
ms := sqlbase.MakeMetadataSchema()
ms := sqlbase.MakeMetadataSchema(config.DefaultZoneConfigRef(), config.DefaultSystemZoneConfigRef())
descIDs := ms.DescriptorIDs()
maxDescID := descIDs[len(descIDs)-1]
kvs, _ /* splits */ := ms.GetInitialValues()
Expand All @@ -188,7 +188,7 @@ func TestGetLargestID(t *testing.T) {
}, 5, 7, ""},
}

cfg := config.NewSystemConfig()
cfg := config.NewSystemConfig(config.DefaultZoneConfigRef())
for tcNum, tc := range testCases {
cfg.Values = tc.values
ret, err := cfg.GetLargestObjectID(tc.maxID)
Expand Down Expand Up @@ -259,8 +259,8 @@ func TestComputeSplitKeySystemRanges(t *testing.T) {
{roachpb.RKey(keys.TimeseriesPrefix.PrefixEnd()), roachpb.RKeyMax, keys.SystemConfigSplitKey},
}

cfg := config.NewSystemConfig()
kvs, _ /* splits */ := sqlbase.MakeMetadataSchema().GetInitialValues()
cfg := config.NewSystemConfig(config.DefaultZoneConfigRef())
kvs, _ /* splits */ := sqlbase.MakeMetadataSchema(cfg.DefaultZoneConfig, config.DefaultSystemZoneConfigRef()).GetInitialValues()
cfg.SystemConfigEntries = config.SystemConfigEntries{
Values: kvs,
}
Expand Down Expand Up @@ -290,7 +290,7 @@ func TestComputeSplitKeyTableIDs(t *testing.T) {
// separately above.
minKey := roachpb.RKey(keys.TimeseriesPrefix.PrefixEnd())

schema := sqlbase.MakeMetadataSchema()
schema := sqlbase.MakeMetadataSchema(config.DefaultZoneConfigRef(), config.DefaultSystemZoneConfigRef())
// Real system tables only.
baseSql, _ /* splits */ := schema.GetInitialValues()
// Real system tables plus some user stuff.
Expand Down Expand Up @@ -369,7 +369,7 @@ func TestComputeSplitKeyTableIDs(t *testing.T) {
{userSQL, tkey(start + 1), tkey(start + 5), nil},
}

cfg := config.NewSystemConfig()
cfg := config.NewSystemConfig(config.DefaultZoneConfigRef())
for tcNum, tc := range testCases {
cfg.Values = tc.values
splitKey := cfg.ComputeSplitKey(tc.start, tc.end)
Expand Down Expand Up @@ -420,9 +420,9 @@ func TestGetZoneConfigForKey(t *testing.T) {
defer func() {
config.ZoneConfigHook = originalZoneConfigHook
}()
cfg := config.NewSystemConfig()
cfg := config.NewSystemConfig(config.DefaultZoneConfigRef())

kvs, _ /* splits */ := sqlbase.MakeMetadataSchema().GetInitialValues()
kvs, _ /* splits */ := sqlbase.MakeMetadataSchema(cfg.DefaultZoneConfig, config.DefaultSystemZoneConfigRef()).GetInitialValues()
cfg.SystemConfigEntries = config.SystemConfigEntries{
Values: kvs,
}
Expand Down
116 changes: 40 additions & 76 deletions pkg/config/zone.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/parser"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/util/protoutil"
"github.com/gogo/protobuf/proto"
"github.com/pkg/errors"
)
Expand Down Expand Up @@ -268,44 +267,6 @@ func (c *Constraint) FromString(short string) error {
// minRangeMaxBytes is the minimum value for range max bytes.
const minRangeMaxBytes = 64 << 10 // 64 KB

// defaultZoneConfig is the default zone configuration used when no custom
// config has been specified.
var defaultZoneConfig = &ZoneConfig{
NumReplicas: proto.Int32(3),
RangeMinBytes: proto.Int64(16 << 20), // 16 MB
RangeMaxBytes: proto.Int64(64 << 20), // 64 MB
GC: &GCPolicy{
// Use 25 hours instead of the previous 24 to make users successful by
// default. Users desiring to take incremental backups every 24h may
// incorrectly assume that the previous default 24h was sufficient to do
// that. But the equation for incremental backups is:
// GC TTLSeconds >= (desired backup interval) + (time to perform incremental backup)
// We think most new users' incremental backups will complete within an
// hour, and larger clusters will have more experienced operators and will
// understand how to change these settings if needed.
TTLSeconds: 25 * 60 * 60,
},
}

// defaultSystemZoneConfig is the default zone configuration used when no custom
// config has been specified for system ranges.
var defaultSystemZoneConfig = &ZoneConfig{
NumReplicas: proto.Int32(5),
RangeMinBytes: proto.Int64(16 << 20), // 16 MB
RangeMaxBytes: proto.Int64(64 << 20), // 64 MB
GC: &GCPolicy{
// Use 25 hours instead of the previous 24 to make users successful by
// default. Users desiring to take incremental backups every 24h may
// incorrectly assume that the previous default 24h was sufficient to do
// that. But the equation for incremental backups is:
// GC TTLSeconds >= (desired backup interval) + (time to perform incremental backup)
// We think most new users' incremental backups will complete within an
// hour, and larger clusters will have more experienced operators and will
// understand how to change these settings if needed.
TTLSeconds: 25 * 60 * 60,
},
}

// NewZoneConfig is the zone configuration used when no custom
// config has been specified.
func NewZoneConfig() *ZoneConfig {
Expand All @@ -331,54 +292,57 @@ func EmptyCompleteZoneConfig() *ZoneConfig {
// DefaultZoneConfig is the default zone configuration used when no custom
// config has been specified.
func DefaultZoneConfig() ZoneConfig {
testingLock.Lock()
defer testingLock.Unlock()
return *protoutil.Clone(defaultZoneConfig).(*ZoneConfig)
return ZoneConfig{
NumReplicas: proto.Int32(3),
RangeMinBytes: proto.Int64(16 << 20), // 16 MB
RangeMaxBytes: proto.Int64(64 << 20), // 64 MB
GC: &GCPolicy{
// Use 25 hours instead of the previous 24 to make users successful by
// default. Users desiring to take incremental backups every 24h may
// incorrectly assume that the previous default 24h was sufficient to do
// that. But the equation for incremental backups is:
// GC TTLSeconds >= (desired backup interval) + (time to perform incremental backup)
// We think most new users' incremental backups will complete within an
// hour, and larger clusters will have more experienced operators and will
// understand how to change these settings if needed.
TTLSeconds: 25 * 60 * 60,
},
}
}

// DefaultZoneConfigRef returns a reference to the default zone config.
// DefaultZoneConfigRef is the default zone configuration used when no custom
// config has been specified.
func DefaultZoneConfigRef() *ZoneConfig {
testingLock.Lock()
defer testingLock.Unlock()
return defaultZoneConfig
zoneConfig := DefaultZoneConfig()
return &zoneConfig
}

// DefaultSystemZoneConfig is the default zone configuration used when no custom
// config has been specified.
func DefaultSystemZoneConfig() ZoneConfig {
testingLock.Lock()
defer testingLock.Unlock()
return *protoutil.Clone(defaultSystemZoneConfig).(*ZoneConfig)
}

// TestingSetDefaultZoneConfig is a testing-only function that changes the
// default zone config and returns a function that reverts the change.
func TestingSetDefaultZoneConfig(cfg ZoneConfig) func() {
testingLock.Lock()
oldConfig := defaultZoneConfig
defaultZoneConfig = &cfg
testingLock.Unlock()

return func() {
testingLock.Lock()
defaultZoneConfig = oldConfig
testingLock.Unlock()
return ZoneConfig{
NumReplicas: proto.Int32(5),
RangeMinBytes: proto.Int64(16 << 20), // 16 MB
RangeMaxBytes: proto.Int64(64 << 20), // 64 MB
GC: &GCPolicy{
// Use 25 hours instead of the previous 24 to make users successful by
// default. Users desiring to take incremental backups every 24h may
// incorrectly assume that the previous default 24h was sufficient to do
// that. But the equation for incremental backups is:
// GC TTLSeconds >= (desired backup interval) + (time to perform incremental backup)
// We think most new users' incremental backups will complete within an
// hour, and larger clusters will have more experienced operators and will
// understand how to change these settings if needed.
TTLSeconds: 25 * 60 * 60,
},
}
}

// TestingSetDefaultSystemZoneConfig is a testing-only function that changes the
// default zone config and returns a function that reverts the change.
func TestingSetDefaultSystemZoneConfig(cfg ZoneConfig) func() {
testingLock.Lock()
oldConfig := defaultSystemZoneConfig
defaultSystemZoneConfig = &cfg
testingLock.Unlock()

return func() {
testingLock.Lock()
defaultSystemZoneConfig = oldConfig
testingLock.Unlock()
}
// DefaultSystemZoneConfigRef is the default zone configuration used when no custom
// config has been specified.
func DefaultSystemZoneConfigRef() *ZoneConfig {
systemZoneConfig := DefaultSystemZoneConfig()
return &systemZoneConfig
}

// IsComplete returns whether all the fields are set.
Expand Down
7 changes: 4 additions & 3 deletions pkg/gossip/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"time"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/config"
"github.com/cockroachdb/cockroach/pkg/gossip/resolver"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/rpc"
Expand Down Expand Up @@ -88,7 +89,7 @@ func startGossipAtAddr(
rpcContext.ClusterID.Reset(clusterID)

server := rpc.NewServer(rpcContext)
g := NewTest(nodeID, rpcContext, server, stopper, registry)
g := NewTest(nodeID, rpcContext, server, stopper, registry, config.DefaultZoneConfigRef())
ln, err := netutil.ListenAndServeGRPC(stopper, server, addr)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -150,7 +151,7 @@ func startFakeServerGossips(
lRPCContext.ClusterID.Reset(clusterID)

lserver := rpc.NewServer(lRPCContext)
local := NewTest(localNodeID, lRPCContext, lserver, stopper, metric.NewRegistry())
local := NewTest(localNodeID, lRPCContext, lserver, stopper, metric.NewRegistry(), config.DefaultZoneConfigRef())
lln, err := netutil.ListenAndServeGRPC(stopper, lserver, util.IsolatedTestAddr)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -513,7 +514,7 @@ func TestClientRegisterWithInitNodeID(t *testing.T) {
server := rpc.NewServer(RPCContext)
// node ID must be non-zero
gnode := NewTest(
nodeID, RPCContext, server, stopper, metric.NewRegistry(),
nodeID, RPCContext, server, stopper, metric.NewRegistry(), config.DefaultZoneConfigRef(),
)
g = append(g, gnode)

Expand Down
Loading

0 comments on commit dd997fd

Please sign in to comment.