add ttl config to disable tikv split #5485

D3Hunter · 2022-09-09T06:11:22Z

What problem does this PR solve?

Issue Number: Close #5486

ref: pingcap/tidb#35489

What is changed and how does it work?

add a ttl configuration to disable tikv split

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
- max region size set to 10k
- insert data until trigger region split
- disable tikv split for 60s
- insert data, see tikv report warning that ErrSchedulerTikvSplitDisabled
- after ttl expired, insert some data, tikv can split normally.
No code

Code changes

Has configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

ti-chi-bot · 2022-09-09T06:11:23Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

nolouch
rleungx

To complete the pull request process, please ask the reviewers in the list 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 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.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: D3Hunter <jujj603@gmail.com>

server/config/persist_options.go

Signed-off-by: D3Hunter <jujj603@gmail.com>

codecov · 2022-09-09T06:58:00Z

Codecov Report

Base: 75.62% // Head: 75.60% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (3cac599) compared to base (5ca01a8).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5485      +/-   ##
==========================================
- Coverage   75.62%   75.60%   -0.02%     
==========================================
  Files         320      320              
  Lines       31622    31640      +18     
==========================================
+ Hits        23913    23923      +10     
- Misses       5645     5660      +15     
+ Partials     2064     2057       -7

Flag	Coverage Δ
unittests	`75.60% <100.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/cluster/cluster_worker.go	`70.00% <100.00%> (+1.03%)`	⬆️
server/config/config.go	`78.22% <100.00%> (+0.09%)`	⬆️
server/config/persist_options.go	`93.64% <100.00%> (+0.26%)`	⬆️
pkg/dashboard/keyvisual/input/core.go	`0.00% <0.00%> (-33.34%)`	⬇️
server/id/id.go	`83.05% <0.00%> (-3.39%)`	⬇️
server/tso/allocator_manager.go	`64.44% <0.00%> (-2.60%)`	⬇️
server/schedule/hbstream/heartbeat_streams.go	`72.72% <0.00%> (-2.03%)`	⬇️
tools/pd-ctl/pdctl/command/operator.go	`66.66% <0.00%> (-1.15%)`	⬇️
pkg/btree/btree.go	`86.26% <0.00%> (-0.97%)`	⬇️
server/schedulers/evict_leader.go	`78.57% <0.00%> (-0.90%)`	⬇️
... and 15 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

JmPotato

The rest LGTM. BTW, Do we need to do some error handling work on the TiKV-side to prevent something like outputting massive split error logs or unnecessary retries?

pkg/errs/errno.go

Signed-off-by: D3Hunter <jujj603@gmail.com>

D3Hunter · 2022-09-09T07:50:56Z

ptal

rleungx · 2022-09-09T08:07:34Z

server/config/persist_options.go

@@ -200,6 +200,12 @@ const (
 	hotRegionScheduleLimitKey      = "schedule.hot-region-schedule-limit"
 	schedulerMaxWaitingOperatorKey = "schedule.scheduler-max-waiting-operator"
 	enableLocationReplacement      = "schedule.enable-location-replacement"
+	// it's related to schedule, but it's not an explicit config
+	enableTiKVSplitRegion = "schedule.enable-tikv-split-region"


Do we need to add it to ScheduleConfig?

not necessary for now, it's not a real config(it's always true except during br operation)

can be deprecated when pd support one-api to pause all schedulers.

I think it's ok to ScheduleConfig, in some scenarios, we also need to disable split. such as recovery

198c7cc , ~~it's visible internally for now(so cannot disable it without ttl)~~

rleungx · 2022-09-09T08:09:23Z

tests/server/cluster/cluster_work_test.go

@@ -100,9 +101,6 @@ func TestAskSplit(t *testing.T) {
 		Region: regions[0].GetMeta(),
 	}

-	_, err = rc.HandleAskSplit(req)


Why remove it?

moved to below lines

see https://github.com/tikv/pd/pull/5485/files/f51c150272c973e1a80f3f297866f8d1082ee6a6#diff-cc382401991df4c6e346c3c68cc95ee7754fed058699bd47a000db7fd005ea1dR121-R122

server/cluster/cluster_worker.go

Signed-off-by: D3Hunter <jujj603@gmail.com>

server/config/config.go

rleungx · 2022-09-13T02:55:47Z

/rebuild

Co-authored-by: Ryan Leung <rleungx@gmail.com> Signed-off-by: D3Hunter <jujj603@gmail.com>

server/config/persist_options.go

server/config/config.go

Signed-off-by: D3Hunter <jujj603@gmail.com>

rleungx

LGTM

nolouch

lgtm

nolouch · 2022-09-13T06:55:03Z

/merge

ti-chi-bot · 2022-09-13T06:55:05Z

@nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-09-13T06:55:06Z

This pull request has been accepted and is ready to merge.

Commit hash: e62eec5

ti-chi-bot added do-not-merge/needs-linked-issue release-note-none labels Sep 9, 2022

ti-chi-bot requested review from disksing and JmPotato September 9, 2022 06:11

ti-chi-bot removed the do-not-merge/needs-linked-issue label Sep 9, 2022

disable tikv split

7b91b9d

Signed-off-by: D3Hunter <jujj603@gmail.com>

D3Hunter force-pushed the disable-split branch from e68d085 to 7b91b9d Compare September 9, 2022 06:15

fix lint

a27b4c7

Signed-off-by: D3Hunter <jujj603@gmail.com>

nolouch reviewed Sep 9, 2022

View reviewed changes

server/config/persist_options.go Outdated Show resolved Hide resolved

fix comments

ad4539f

Signed-off-by: D3Hunter <jujj603@gmail.com>

JmPotato reviewed Sep 9, 2022

View reviewed changes

pkg/errs/errno.go Outdated Show resolved Hide resolved

fix comments

0e599a4

Signed-off-by: D3Hunter <jujj603@gmail.com>

D3Hunter force-pushed the disable-split branch from b98df2b to 0e599a4 Compare September 9, 2022 07:21

add ut

f51c150

Signed-off-by: D3Hunter <jujj603@gmail.com>

rleungx reviewed Sep 9, 2022

View reviewed changes

server/cluster/cluster_worker.go Outdated Show resolved Hide resolved

D3Hunter added 6 commits September 9, 2022 18:00

fix comments

2440b91

Signed-off-by: D3Hunter <jujj603@gmail.com>

make it named config

c878aba

Signed-off-by: D3Hunter <jujj603@gmail.com>

make it toml invisible

198c7cc

Signed-off-by: D3Hunter <jujj603@gmail.com>

expose it

7398dbb

Signed-off-by: D3Hunter <jujj603@gmail.com>

fix lint

a7a50d5

Signed-off-by: D3Hunter <jujj603@gmail.com>

fix lint

97efbc4

Signed-off-by: D3Hunter <jujj603@gmail.com>

rleungx reviewed Sep 13, 2022

View reviewed changes

server/config/config.go Outdated Show resolved Hide resolved

Update server/config/config.go

a8ddbbb

Co-authored-by: Ryan Leung <rleungx@gmail.com> Signed-off-by: D3Hunter <jujj603@gmail.com>

D3Hunter force-pushed the disable-split branch from e1a1353 to a8ddbbb Compare September 13, 2022 03:03

rleungx reviewed Sep 13, 2022

View reviewed changes

server/config/persist_options.go Show resolved Hide resolved

server/config/config.go Outdated Show resolved Hide resolved

D3Hunter added 2 commits September 13, 2022 13:25

ut

714095c

Signed-off-by: D3Hunter <jujj603@gmail.com>

ut

e62eec5

Signed-off-by: D3Hunter <jujj603@gmail.com>

rleungx approved these changes Sep 13, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 13, 2022

nolouch approved these changes Sep 13, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 13, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 13, 2022

Merge branch 'master' into disable-split

3cac599

ti-chi-bot merged commit 86262da into tikv:master Sep 13, 2022

D3Hunter deleted the disable-split branch September 13, 2022 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ttl config to disable tikv split #5485

add ttl config to disable tikv split #5485

D3Hunter commented Sep 9, 2022 •

edited

ti-chi-bot commented Sep 9, 2022 •

edited

codecov bot commented Sep 9, 2022 •

edited

JmPotato left a comment

D3Hunter commented Sep 9, 2022

rleungx Sep 9, 2022

D3Hunter Sep 9, 2022

nolouch Sep 9, 2022

D3Hunter Sep 9, 2022 •

edited

rleungx Sep 9, 2022

D3Hunter Sep 9, 2022

D3Hunter Sep 9, 2022

rleungx commented Sep 13, 2022

rleungx left a comment

nolouch left a comment

nolouch commented Sep 13, 2022

ti-chi-bot commented Sep 13, 2022

ti-chi-bot commented Sep 13, 2022

add ttl config to disable tikv split #5485

add ttl config to disable tikv split #5485

Conversation

D3Hunter commented Sep 9, 2022 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Sep 9, 2022 • edited

codecov bot commented Sep 9, 2022 • edited

Codecov Report

JmPotato left a comment

Choose a reason for hiding this comment

D3Hunter commented Sep 9, 2022

rleungx Sep 9, 2022

Choose a reason for hiding this comment

D3Hunter Sep 9, 2022

Choose a reason for hiding this comment

nolouch Sep 9, 2022

Choose a reason for hiding this comment

D3Hunter Sep 9, 2022 • edited

Choose a reason for hiding this comment

rleungx Sep 9, 2022

Choose a reason for hiding this comment

D3Hunter Sep 9, 2022

Choose a reason for hiding this comment

D3Hunter Sep 9, 2022

Choose a reason for hiding this comment

rleungx commented Sep 13, 2022

rleungx left a comment

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

nolouch commented Sep 13, 2022

ti-chi-bot commented Sep 13, 2022

ti-chi-bot commented Sep 13, 2022

D3Hunter commented Sep 9, 2022 •

edited

ti-chi-bot commented Sep 9, 2022 •

edited

codecov bot commented Sep 9, 2022 •

edited

D3Hunter Sep 9, 2022 •

edited