Repository: tikv/pd
Pull request #3747 (2021-06-04 10:52:04)

*: make flow-round-by-digit compatible with trace-region-flow
Signed-off-by: nolouch <nolouch@gmail.com>

<!--
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?

deprecate `trace-region-flow`

### What is changed and how it works?

### Check List

<!-- Remove the items that are not applicable. -->

Tests

<!-- At least one of them must be included. -->

- Unit test
- Integration test
- Manual test (add detailed scripts or steps below)
situation 1
```
### old server
➜  pd git:(remove-trace-flow) ✗ ./bin/pd-ctl config set trace-region-flow false
Success!
➜  pd git:(remove-trace-flow) ✗ ./bin/pd-ctl config show all |grep flow        
"trace-region-flow": "false"

### start new binary
➜  pd git:(remove-trace-flow) ✗ ./bin/pd-ctl config show all |grep flow
"flow-round-by-digit": 9223372036854775807
➜  pd git:(remove-trace-flow) ✗ ./bin/pd-ctl config set flow-round-by-digit 9
Success!
➜  pd git:(remove-trace-flow) ✗ ./bin/pd-ctl config show all |grep flow      
"flow-round-by-digit": 9
### restart new binary
➜  pd git:(remove-trace-flow) ✗ ./bin/pd-ctl config show all |grep flow      
"flow-round-by-digit": 9
```

situation 2
```
### old server
➜  pd git:(remove-trace-flow) ./bin/pd-ctl config show all |grep flow
    "trace-region-flow": "true"
### restart with new binary
➜  pd git:(remove-trace-flow) ./bin/pd-ctl config show all |grep flow
    "flow-round-by-digit": 3
### restart with new binary
➜  pd git:(remove-trace-flow) ./bin/pd-ctl config show all |grep flow
    "flow-round-by-digit": 3
```
### Release note


```release-note
 Deprecate  `trace-region-flow`, and use new config `flow-round-by-digit`
```


ti-chi-bot: [REVIEW NOTIFICATION]

This pull request has not been approved.


To complete the [pull request process](https://book.prow.tidb.io/#/en/workflows/pr), please ask the reviewers in the [list](https://prow.tidb.io/tichi/repos/tikv/pd/pulls/3747/owners) to review by filling `/cc @reviewer` in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the [list](https://prow.tidb.io/tichi/repos/tikv/pd/pulls/3747/owners) by filling  `/assign @committer` in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found [here](https://prow.tidb.io/command-help?repo=tikv%2Fpd).

<details>

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.
</details>

<!--Review Notification Identifier-->
codecov[bot]: # [Codecov](https://codecov.io/gh/tikv/pd/pull/3747?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv) Report
> Merging [#3747](https://codecov.io/gh/tikv/pd/pull/3747?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv) (f387675) into [master](https://codecov.io/gh/tikv/pd/commit/a9e629d3b86d1e841c475aa5eb7c6a1b9ebf804f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv) (a9e629d) will **increase** coverage by `0.09%`.
> The diff coverage is `70.58%`.

[![Impacted file tree graph](https://codecov.io/gh/tikv/pd/pull/3747/graphs/tree.svg?width=650&height=150&src=pr&token=J1yRL471OI&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv)](https://codecov.io/gh/tikv/pd/pull/3747?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv)

```diff
@@            Coverage Diff             @@
##           master    #3747      +/-   ##
==========================================
+ Coverage   75.10%   75.20%   +0.09%     
==========================================
  Files         244      244              
  Lines       24477    24492      +15     
==========================================
+ Hits        18384    18418      +34     
+ Misses       4472     4460      -12     
+ Partials     1621     1614       -7     
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `75.20% <70.58%> (+0.09%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/tikv/pd/pull/3747?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv) | Coverage Δ | |
|---|---|---|
| [server/config/config.go](https://codecov.io/gh/tikv/pd/pull/3747/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv#diff-c2VydmVyL2NvbmZpZy9jb25maWcuZ28=) | `74.84% <64.28%> (-0.32%)` | :arrow_down: |
| [server/cluster/cluster.go](https://codecov.io/gh/tikv/pd/pull/3747/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv#diff-c2VydmVyL2NsdXN0ZXIvY2x1c3Rlci5nbw==) | `82.76% <100.00%> (ø)` | |
| [server/config/persist\_options.go](https://codecov.io/gh/tikv/pd/pull/3747/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv#diff-c2VydmVyL2NvbmZpZy9wZXJzaXN0X29wdGlvbnMuZ28=) | `91.40% <100.00%> (+0.03%)` | :arrow_up: |
| [server/schedulers/utils.go](https://codecov.io/gh/tikv/pd/pull/3747/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv#diff-c2VydmVyL3NjaGVkdWxlcnMvdXRpbHMuZ28=) | `93.75% <0.00%> (-1.05%)` | :arrow_down: |
| [pkg/btree/btree.go](https://codecov.io/gh/tikv/pd/pull/3747/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv#diff-cGtnL2J0cmVlL2J0cmVlLmdv) | `85.30% <0.00%> (-0.97%)` | :arrow_down: |
| [server/schedule/operator/step.go](https://codecov.io/gh/tikv/pd/pull/3747/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv#diff-c2VydmVyL3NjaGVkdWxlL29wZXJhdG9yL3N0ZXAuZ28=) | `67.77% <0.00%> (-0.67%)` | :arrow_down: |
| [server/grpc\_service.go](https://codecov.io/gh/tikv/pd/pull/3747/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv#diff-c2VydmVyL2dycGNfc2VydmljZS5nbw==) | `48.57% <0.00%> (-0.23%)` | :arrow_down: |
| [server/tso/allocator\_manager.go](https://codecov.io/gh/tikv/pd/pull/3747/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv#diff-c2VydmVyL3Rzby9hbGxvY2F0b3JfbWFuYWdlci5nbw==) | `79.13% <0.00%> (+0.16%)` | :arrow_up: |
| [server/server.go](https://codecov.io/gh/tikv/pd/pull/3747/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv#diff-c2VydmVyL3NlcnZlci5nbw==) | `72.07% <0.00%> (+0.75%)` | :arrow_up: |
| [server/tso/tso.go](https://codecov.io/gh/tikv/pd/pull/3747/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv#diff-c2VydmVyL3Rzby90c28uZ28=) | `74.15% <0.00%> (+4.49%)` | :arrow_up: |
| ... and [3 more](https://codecov.io/gh/tikv/pd/pull/3747/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/tikv/pd/pull/3747?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/tikv/pd/pull/3747?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv). Last update [a9e629d...f387675](https://codecov.io/gh/tikv/pd/pull/3747?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tikv).

ti-chi-bot: [REVIEW NOTIFICATION]

This pull request has been approved by:

- lhy1024




To complete the [pull request process](https://book.prow.tidb.io/#/en/workflows/pr), please ask the reviewers in the [list](https://prow.tidb.io/tichi/repos/tikv/pd/pulls/3747/owners) to review by filling `/cc @reviewer` in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the [list](https://prow.tidb.io/tichi/repos/tikv/pd/pulls/3747/owners) by filling  `/assign @committer` in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found [here](https://prow.tidb.io/command-help?repo=tikv%2Fpd).

<details>

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.
</details>

<!--Review Notification Identifier-->
ti-chi-bot: [REVIEW NOTIFICATION]

This pull request has been approved by:

- lhy1024
- rleungx




To complete the [pull request process](https://book.prow.tidb.io/#/en/workflows/pr), please ask the reviewers in the [list](https://prow.tidb.io/tichi/repos/tikv/pd/pulls/3747/owners) to review by filling `/cc @reviewer` in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the [list](https://prow.tidb.io/tichi/repos/tikv/pd/pulls/3747/owners) by filling  `/assign @committer` in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found [here](https://prow.tidb.io/command-help?repo=tikv%2Fpd).

<details>

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.
</details>

<!--Review Notification Identifier-->
nolouch: /merge
ti-chi-bot: @nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests


<details>

Instructions for interacting with me using PR comments are available [here](https://prow.tidb.io/command-help).  If you have questions or suggestions related to my behavior, please file an issue against the [ti-community-infra/tichi](https://github.com/ti-community-infra/tichi/issues/new?title=Prow%20issue:) repository.
</details>
ti-chi-bot: This pull request has been accepted and is ready to merge. <details>Commit hash: f387675e9f3f880a3f708a66808bfa95400d55ff</details>

Patch:
diff --git a/server/cluster/cluster.go b/server/cluster/cluster.go
index 9f1b05678..922aa8f68 100644
--- a/server/cluster/cluster.go
+++ b/server/cluster/cluster.go
@@ -659,8 +659,8 @@ func (c *RaftCluster) processRegionHeartbeat(region *core.RegionInfo) error {
 		}
 		// Once flow has changed, will update the cache.
 		// Because keys and bytes are strongly related, only bytes are judged.
-		if c.traceRegionFlow && (region.GetRoundBytesWritten() != origin.GetRoundBytesWritten() ||
-			region.GetRoundBytesRead() != origin.GetRoundBytesRead()) {
+		if region.GetRoundBytesWritten() != origin.GetRoundBytesWritten() ||
+			region.GetRoundBytesRead() != origin.GetRoundBytesRead() {
 			saveCache, needSync = true, true
 		}
 
diff --git a/server/config/config.go b/server/config/config.go
index d83486f53..cf063ab82 100644
--- a/server/config/config.go
+++ b/server/config/config.go
@@ -19,6 +19,7 @@ import (
 	"encoding/json"
 	"flag"
 	"fmt"
+	"math"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -1076,9 +1077,9 @@ type PDServerConfig struct {
 	MetricStorage string `toml:"metric-storage" json:"metric-storage"`
 	// There are some values supported: "auto", "none", or a specific address, default: "auto"
 	DashboardAddress string `toml:"dashboard-address" json:"dashboard-address"`
-	// TraceRegionFlow the option to update flow information of regions
-	// TODO: deprecate
-	TraceRegionFlow bool `toml:"trace-region-flow" json:"trace-region-flow,string"`
+	// TraceRegionFlow the option to update flow information of regions.
+	// WARN: TraceRegionFlow is deprecated.
+	TraceRegionFlow bool `toml:"trace-region-flow" json:"trace-region-flow,string,omitempty"`
 	// FlowRoundByDigit used to discretization processing flow information.
 	FlowRoundByDigit int `toml:"flow-round-by-digit" json:"flow-round-by-digit"`
 }
@@ -1103,9 +1104,35 @@ func (c *PDServerConfig) adjust(meta *configMetaData) error {
 	if !meta.IsDefined("flow-round-by-digit") {
 		adjustInt(&c.FlowRoundByDigit, defaultFlowRoundByDigit)
 	}
+	c.migrateConfigurationFromFile(meta)
 	return c.Validate()
 }
 
+func (c *PDServerConfig) migrateConfigurationFromFile(meta *configMetaData) error {
+	oldName, newName := "trace-region-flow", "flow-round-by-digit"
+	defineOld, defineNew := meta.IsDefined(oldName), meta.IsDefined(newName)
+	switch {
+	case defineOld && defineNew:
+		if c.TraceRegionFlow && (c.FlowRoundByDigit == defaultFlowRoundByDigit) {
+			return errors.Errorf("config item %s and %s(deprecated) are conflict", newName, oldName)
+		}
+	case defineOld && !defineNew:
+		if !c.TraceRegionFlow {
+			c.FlowRoundByDigit = math.MaxInt8
+		}
+	}
+	return nil
+}
+
+// MigrateDeprecatedFlags updates new flags according to deprecated flags.
+func (c *PDServerConfig) MigrateDeprecatedFlags() {
+	if !c.TraceRegionFlow {
+		c.FlowRoundByDigit = math.MaxInt8
+	}
+	// json omity the false. next time will not persist to the kv.
+	c.TraceRegionFlow = false
+}
+
 // Clone returns a cloned PD server config.
 func (c *PDServerConfig) Clone() *PDServerConfig {
 	runtimeServices := append(c.RuntimeServices[:0:0], c.RuntimeServices...)
diff --git a/server/config/persist_options.go b/server/config/persist_options.go
index 63a1b75f8..ca2660f62 100644
--- a/server/config/persist_options.go
+++ b/server/config/persist_options.go
@@ -580,6 +580,7 @@ func (o *PersistOptions) Reload(storage *core.Storage) error {
 		return err
 	}
 	o.adjustScheduleCfg(&cfg.Schedule)
+	cfg.PDServerCfg.MigrateDeprecatedFlags()
 	if isExist {
 		o.schedule.Store(&cfg.Schedule)
 		o.replication.Store(&cfg.Replication)


Test patch:
diff --git a/server/cluster/cluster_test.go b/server/cluster/cluster_test.go
index ed785d086..317d06b71 100644
--- a/server/cluster/cluster_test.go
+++ b/server/cluster/cluster_test.go
@@ -622,13 +622,6 @@ func (s *testClusterInfoSuite) TestRegionFlowChanged(c *C) {
 	processRegions(regions)
 	newRegion := cluster.GetRegion(region.GetID())
 	c.Assert(newRegion.GetBytesRead(), Equals, uint64(1000))
-
-	// do not trace the flow changes
-	cluster.traceRegionFlow = false
-	processRegions([]*core.RegionInfo{region})
-	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 be4fc887f..2f36dcd24 100644
--- a/server/config/config_test.go
+++ b/server/config/config_test.go
@@ -16,6 +16,7 @@ package config
 import (
 	"encoding/json"
 	"fmt"
+	"math"
 	"os"
 	"path"
 	"strings"
@@ -284,6 +285,8 @@ func (s *testConfigSuite) TestMigrateFlags(c *C) {
 		return cfg, err
 	}
 	cfg, err := load(`
+[pd-server]
+trace-region-flow = false
 [schedule]
 disable-remove-down-replica = true
 enable-make-up-replica = false
@@ -291,6 +294,7 @@ disable-remove-extra-replica = true
 enable-remove-extra-replica = false
 `)
 	c.Assert(err, IsNil)
+	c.Assert(cfg.PDServerCfg.FlowRoundByDigit, Equals, math.MaxInt8)
 	c.Assert(cfg.Schedule.EnableReplaceOfflineReplica, IsTrue)
 	c.Assert(cfg.Schedule.EnableRemoveDownReplica, IsFalse)
 	c.Assert(cfg.Schedule.EnableMakeUpReplica, IsFalse)