Repository: tikv/pd
Pull request #3116 (2020-10-28 08:08:50)

config: refine config clone
<!--
Thank you for working on PD! Please read PD's [CONTRIBUTING](https://github.com/tikv/pd/blob/master/CONTRIBUTING.md) document **BEFORE** filing this PR.
PR Title Format:
1. pkg [, pkg2, pkg3]: what's changed
2. *: what's changed


If you want to open the **Challenge Program** pull request, please use the following template:
https://raw.githubusercontent.com/tikv/.github/master/.github/PULL_REQUEST_TEMPLATE/challenge-program.md
You can use it with query parameters: https://github.com/tikv/pd/compare/master...${you branch}?template=challenge-program.md
-->

### What problem does this PR solve?

<!-- Add the issue link with a summary if it exists. -->

* The config of `store-balance-rate` and `trace-region-flow` will be lost during `Clone`.

### What is changed and how it works?

* config: refine config clone
* server: clone config in `GetConfig`

### Check List

Related changes

- Need to cherry-pick to the release branch

### Release note

<!-- A bugfix or a new feature needs a release note. If there is no need release note, just uncomment the below line. -->

- Fix the bug that `trace-region-flow` will be accidentally changed to `false`.


nolouch: LGTM
HunDunDM: > Basically LGTM. Can you add unit test to verify it?

A little difficult. I removed the enumeration in the `Clone` to prevent omissions. But if you add tests, you still need to enumerate all the member variables.
rleungx: CI failed due to `SetStoreLimit`
HunDunDM: /run-all-tests
nolouch: /merge
ti-srebot: /run-all-tests
ti-srebot: @HunDunDM merge failed.
HunDunDM: /merge
ti-srebot: /run-all-tests

Patch:
diff --git a/server/config/config.go b/server/config/config.go
index 8c8be51c9..88d29a0ea 100644
--- a/server/config/config.go
+++ b/server/config/config.go
@@ -578,9 +578,8 @@ func (c *Config) adjustLog(meta *configMetaData) {
 
 // Clone returns a cloned configuration.
 func (c *Config) Clone() *Config {
-	cfg := &Config{}
-	*cfg = *c
-	return cfg
+	cfg := *c
+	return &cfg
 }
 
 func (c *Config) String() string {
@@ -713,50 +712,19 @@ type ScheduleConfig struct {
 
 // Clone returns a cloned scheduling configuration.
 func (c *ScheduleConfig) Clone() *ScheduleConfig {
-	schedulers := make(SchedulerConfigs, len(c.Schedulers))
-	copy(schedulers, c.Schedulers)
-	storeLimit := make(map[uint64]StoreLimitConfig, len(c.StoreLimit))
-	for k, v := range c.StoreLimit {
-		storeLimit[k] = v
-	}
-	return &ScheduleConfig{
-		MaxSnapshotCount:             c.MaxSnapshotCount,
-		MaxPendingPeerCount:          c.MaxPendingPeerCount,
-		MaxMergeRegionSize:           c.MaxMergeRegionSize,
-		MaxMergeRegionKeys:           c.MaxMergeRegionKeys,
-		SplitMergeInterval:           c.SplitMergeInterval,
-		PatrolRegionInterval:         c.PatrolRegionInterval,
-		MaxStoreDownTime:             c.MaxStoreDownTime,
-		LeaderScheduleLimit:          c.LeaderScheduleLimit,
-		LeaderSchedulePolicy:         c.LeaderSchedulePolicy,
-		RegionScheduleLimit:          c.RegionScheduleLimit,
-		ReplicaScheduleLimit:         c.ReplicaScheduleLimit,
-		MergeScheduleLimit:           c.MergeScheduleLimit,
-		EnableOneWayMerge:            c.EnableOneWayMerge,
-		EnableCrossTableMerge:        c.EnableCrossTableMerge,
-		HotRegionScheduleLimit:       c.HotRegionScheduleLimit,
-		HotRegionCacheHitsThreshold:  c.HotRegionCacheHitsThreshold,
-		StoreLimit:                   storeLimit,
-		TolerantSizeRatio:            c.TolerantSizeRatio,
-		LowSpaceRatio:                c.LowSpaceRatio,
-		HighSpaceRatio:               c.HighSpaceRatio,
-		SchedulerMaxWaitingOperator:  c.SchedulerMaxWaitingOperator,
-		DisableLearner:               c.DisableLearner,
-		DisableRemoveDownReplica:     c.DisableRemoveDownReplica,
-		DisableReplaceOfflineReplica: c.DisableReplaceOfflineReplica,
-		DisableMakeUpReplica:         c.DisableMakeUpReplica,
-		DisableRemoveExtraReplica:    c.DisableRemoveExtraReplica,
-		DisableLocationReplacement:   c.DisableLocationReplacement,
-		EnableRemoveDownReplica:      c.EnableRemoveDownReplica,
-		EnableReplaceOfflineReplica:  c.EnableReplaceOfflineReplica,
-		EnableMakeUpReplica:          c.EnableMakeUpReplica,
-		EnableRemoveExtraReplica:     c.EnableRemoveExtraReplica,
-		EnableLocationReplacement:    c.EnableLocationReplacement,
-		EnableDebugMetrics:           c.EnableDebugMetrics,
-		EnableJointConsensus:         c.EnableJointConsensus,
-		StoreLimitMode:               c.StoreLimitMode,
-		Schedulers:                   schedulers,
+	schedulers := append(c.Schedulers[:0:0], c.Schedulers...)
+	var storeLimit map[uint64]StoreLimitConfig
+	if c.StoreLimit != nil {
+		storeLimit = make(map[uint64]StoreLimitConfig, len(c.StoreLimit))
+		for k, v := range c.StoreLimit {
+			storeLimit[k] = v
+		}
 	}
+	cfg := *c
+	cfg.StoreLimit = storeLimit
+	cfg.Schedulers = schedulers
+	cfg.SchedulersPayload = nil
+	return &cfg
 }
 
 const (
@@ -852,6 +820,10 @@ func (c *ScheduleConfig) adjust(meta *configMetaData) error {
 		c.StoreBalanceRate = 0
 	}
 
+	if c.StoreLimit == nil {
+		c.StoreLimit = make(map[uint64]StoreLimitConfig)
+	}
+
 	return c.Validate()
 }
 
@@ -1013,15 +985,10 @@ type ReplicationConfig struct {
 
 // Clone makes a deep copy of the config.
 func (c *ReplicationConfig) Clone() *ReplicationConfig {
-	locationLabels := make(typeutil.StringSlice, len(c.LocationLabels))
-	copy(locationLabels, c.LocationLabels)
-	return &ReplicationConfig{
-		MaxReplicas:          c.MaxReplicas,
-		LocationLabels:       locationLabels,
-		StrictlyMatchLabel:   c.StrictlyMatchLabel,
-		EnablePlacementRules: c.EnablePlacementRules,
-		IsolationLevel:       c.IsolationLevel,
-	}
+	locationLabels := append(c.LocationLabels[:0:0], c.LocationLabels...)
+	cfg := *c
+	cfg.LocationLabels = locationLabels
+	return &cfg
 }
 
 // Validate is used to validate if some replication configurations are right.
@@ -1099,16 +1066,10 @@ func (c *PDServerConfig) adjust(meta *configMetaData) error {
 
 // Clone returns a cloned PD server config.
 func (c *PDServerConfig) Clone() *PDServerConfig {
-	runtimeServices := make(typeutil.StringSlice, len(c.RuntimeServices))
-	copy(runtimeServices, c.RuntimeServices)
-	return &PDServerConfig{
-		UseRegionStorage: c.UseRegionStorage,
-		MaxResetTSGap:    c.MaxResetTSGap,
-		KeyType:          c.KeyType,
-		MetricStorage:    c.MetricStorage,
-		DashboardAddress: c.DashboardAddress,
-		RuntimeServices:  runtimeServices,
-	}
+	runtimeServices := append(c.RuntimeServices[:0:0], c.RuntimeServices...)
+	cfg := *c
+	cfg.RuntimeServices = runtimeServices
+	return &cfg
 }
 
 // Validate is used to validate if some pd-server configurations are right.
diff --git a/server/server.go b/server/server.go
index 3ff36d4d9..e5033af58 100644
--- a/server/server.go
+++ b/server/server.go
@@ -729,9 +729,9 @@ func (s *Server) StartTimestamp() int64 {
 // GetConfig gets the config information.
 func (s *Server) GetConfig() *config.Config {
 	cfg := s.cfg.Clone()
-	cfg.Schedule = *s.persistOptions.GetScheduleConfig()
-	cfg.Replication = *s.persistOptions.GetReplicationConfig()
-	cfg.PDServerCfg = *s.persistOptions.GetPDServerConfig()
+	cfg.Schedule = *s.persistOptions.GetScheduleConfig().Clone()
+	cfg.Replication = *s.persistOptions.GetReplicationConfig().Clone()
+	cfg.PDServerCfg = *s.persistOptions.GetPDServerConfig().Clone()
 	cfg.ReplicationMode = *s.persistOptions.GetReplicationModeConfig()
 	cfg.LabelProperty = s.persistOptions.GetLabelPropertyConfig().Clone()
 	cfg.ClusterVersion = *s.persistOptions.GetClusterVersion()


Test patch:
diff --git a/server/cluster/cluster_test.go b/server/cluster/cluster_test.go
index ed1d0688d..c510ca4aa 100644
--- a/server/cluster/cluster_test.go
+++ b/server/cluster/cluster_test.go
@@ -375,7 +375,6 @@ func (s *testClusterInfoSuite) TestRegionFlowChanged(c *C) {
 	newRegion = cluster.GetRegion(region.GetID())
 	c.Assert(region.GetBytesRead(), Equals, uint64(0))
 	c.Assert(newRegion.GetBytesRead(), Not(Equals), uint64(0))
-
 }
 
 func (s *testClusterInfoSuite) TestConcurrentRegionHeartbeat(c *C) {
diff --git a/server/config/config_test.go b/server/config/config_test.go
index 3fc05e758..b695d4cf6 100644
--- a/server/config/config_test.go
+++ b/server/config/config_test.go
@@ -434,3 +434,27 @@ wait-store-timeout = "120s"
 	c.Assert(err, IsNil)
 	c.Assert(cfg.ReplicationMode.ReplicationMode, Equals, "majority")
 }
+
+func (s *testConfigSuite) TestConfigClone(c *C) {
+	cfg := &Config{}
+	cfg.Adjust(nil)
+	c.Assert(cfg.Clone(), DeepEquals, cfg)
+
+	emptyConfigMetaData := newConfigMetadata(nil)
+
+	schedule := &ScheduleConfig{}
+	schedule.adjust(emptyConfigMetaData)
+	c.Assert(schedule.Clone(), DeepEquals, schedule)
+
+	replication := &ReplicationConfig{}
+	replication.adjust(emptyConfigMetaData)
+	c.Assert(replication.Clone(), DeepEquals, replication)
+
+	pdServer := &PDServerConfig{}
+	pdServer.adjust(emptyConfigMetaData)
+	c.Assert(pdServer.Clone(), DeepEquals, pdServer)
+
+	replicationMode := &ReplicationModeConfig{}
+	replicationMode.adjust(emptyConfigMetaData)
+	c.Assert(replicationMode.Clone(), DeepEquals, replicationMode)
+}