*: use a separate runner for updating subtree #8158

rleungx · 2024-05-09T03:28:47Z

What problem does this PR solve?

Issue Number: Ref #7897

What is changed and how does it work?

Check List

Tests

Manual test

Release note

None.

ti-chi-bot · 2024-05-09T03:28:50Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
nolouch

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.

codecov · 2024-05-09T06:01:16Z

Codecov Report

Attention: Patch coverage is 93.33333% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 77.38%. Comparing base (7731ee4) to head (eae4f6a).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8158      +/-   ##
==========================================
+ Coverage   77.34%   77.38%   +0.03%     
==========================================
  Files         471      471              
  Lines       61398    61407       +9     
==========================================
+ Hits        47489    47520      +31     
+ Misses      10350    10325      -25     
- Partials     3559     3562       +3

Flag	Coverage Δ
unittests	`77.38% <93.33%> (+0.03%)`	⬆️

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

pkg/ratelimit/metrics.go

pkg/ratelimit/runner.go

JmPotato · 2024-05-13T04:24:18Z

pkg/ratelimit/metrics.go

+	prometheus.MustRegister(RunnerPendingTasks)
+	prometheus.MustRegister(RunnerFailedTasks)
+	prometheus.MustRegister(RunnerTaskExecutionDuration)
+	prometheus.MustRegister(RunnerSucceededTasks)


Should we also add metrics to tell the task number of different priorities?

pkg/ratelimit/runner.go

rleungx · 2024-05-16T03:55:04Z

server/cluster/cluster.go

@@ -1121,7 +1122,7 @@ func (c *RaftCluster) processRegionHeartbeat(ctx *core.MetaProcessContext, regio
 	tracer.OnCollectRegionStatsFinished()
 	if c.storage != nil {
 		if saveKV {
-			ctx.TaskRunner.RunTask(
+			ctx.StatisticsRunner.RunTask(


SaveRegionToKV is put into the statistics runner, the runner name might be strange. But I cannot come up with a better name.

How about just calling it MiscRunner?

Signed-off-by: Ryan Leung <rleungx@gmail.com>

nolouch · 2024-05-17T06:04:05Z

pkg/mcs/scheduling/server/cluster.go

 	if c.persistConfig.GetScheduleConfig().EnableHeartbeatConcurrentRunner {
-		taskRunner = c.heartbeatRunnner
+		taskRunner = c.heartbeatRunner


need to add in here?

statisticsRunner = c.statisticsRunner

nolouch

rest lgtm

Signed-off-by: Ryan Leung <rleungx@gmail.com>

JmPotato · 2024-05-20T03:20:43Z

pkg/core/context.go

+	TaskRunner       ratelimit.Runner
+	StatisticsRunner ratelimit.Runner
+	LogRunner        ratelimit.Runner


Could you please help add some comments here to explain the different responsibilities of these runners?

JmPotato · 2024-05-20T03:23:09Z

pkg/ratelimit/runner.go

 	f           func(context.Context)
 	name        string
+	retained    bool


This naming is vague without looking at the actual logic, maybe a line of comment would be helpful.

JmPotato · 2024-05-20T03:24:31Z

server/cluster/cluster.go

@@ -1121,7 +1122,7 @@ func (c *RaftCluster) processRegionHeartbeat(ctx *core.MetaProcessContext, regio
 	tracer.OnCollectRegionStatsFinished()
 	if c.storage != nil {
 		if saveKV {
-			ctx.TaskRunner.RunTask(
+			ctx.StatisticsRunner.RunTask(


How about just calling it MiscRunner?

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx · 2024-05-21T09:57:57Z

/merge

ti-chi-bot · 2024-05-21T09:57:58Z

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

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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 · 2024-05-21T09:58:00Z

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

Commit hash: eae4f6a

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

ti-chi-bot bot requested review from lhy1024 and nolouch May 9, 2024 03:28

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/needs-linked-issue labels May 9, 2024

rleungx force-pushed the queue branch from 1658f1f to 9d375e1 Compare May 9, 2024 05:48

rleungx requested review from JmPotato and removed request for lhy1024 May 9, 2024 06:51

nolouch reviewed May 9, 2024

View reviewed changes

pkg/ratelimit/metrics.go Show resolved Hide resolved

pkg/ratelimit/runner.go Outdated Show resolved Hide resolved

rleungx force-pushed the queue branch from 9d375e1 to 41d17db Compare May 13, 2024 04:18

JmPotato reviewed May 13, 2024

View reviewed changes

rleungx requested review from JmPotato and nolouch May 13, 2024 07:07

nolouch reviewed May 13, 2024

View reviewed changes

pkg/ratelimit/runner.go Show resolved Hide resolved

nolouch reviewed May 14, 2024

View reviewed changes

pkg/ratelimit/runner.go Outdated Show resolved Hide resolved

pkg/ratelimit/runner.go Outdated Show resolved Hide resolved

rleungx force-pushed the queue branch from 09cc309 to 3f47763 Compare May 15, 2024 10:10

rleungx changed the title *: distinguish the task priority *: use a separate runner for updating subtree May 16, 2024

rleungx commented May 16, 2024

View reviewed changes

rleungx force-pushed the queue branch from 3f5723d to 129ad25 Compare May 16, 2024 05:54

rleungx added 4 commits May 17, 2024 13:32

distinguish the task priority

c0f2c94

Signed-off-by: Ryan Leung <rleungx@gmail.com>

prevent oom

43c60cb

Signed-off-by: Ryan Leung <rleungx@gmail.com>

address comments

d458613

Signed-off-by: Ryan Leung <rleungx@gmail.com>

limit the count of high priority queue

835257a

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx added 5 commits May 17, 2024 13:32

address comments

1c4336a

Signed-off-by: Ryan Leung <rleungx@gmail.com>

address comments

1b64ad0

Signed-off-by: Ryan Leung <rleungx@gmail.com>

split statistics runner

0f349a0

Signed-off-by: Ryan Leung <rleungx@gmail.com>

remove high priority task queue

2004b2d

Signed-off-by: Ryan Leung <rleungx@gmail.com>

tiny change

61951f2

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the queue branch from 129ad25 to 61951f2 Compare May 17, 2024 05:33

rleungx requested a review from nolouch May 17, 2024 05:56

nolouch reviewed May 17, 2024

View reviewed changes

tiny fix

62d98f4

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx requested a review from nolouch May 17, 2024 06:11

nolouch approved these changes May 17, 2024

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 17, 2024

JmPotato reviewed May 20, 2024

View reviewed changes

add more comments and rename

ed08415

Signed-off-by: Ryan Leung <rleungx@gmail.com>

JmPotato approved these changes May 21, 2024

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 21, 2024

Merge branch 'master' into queue

eae4f6a

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label May 21, 2024

ti-chi-bot bot merged commit 58e7580 into tikv:master May 21, 2024
19 checks passed

rleungx deleted the queue branch May 21, 2024 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: use a separate runner for updating subtree #8158

*: use a separate runner for updating subtree #8158

rleungx commented May 9, 2024 •

edited

ti-chi-bot bot commented May 9, 2024 •

edited

codecov bot commented May 9, 2024 •

edited

JmPotato May 13, 2024

rleungx May 16, 2024

JmPotato May 20, 2024

nolouch May 17, 2024

rleungx May 17, 2024

nolouch left a comment

JmPotato May 20, 2024

JmPotato May 20, 2024

JmPotato May 20, 2024

rleungx commented May 21, 2024

ti-chi-bot bot commented May 21, 2024

ti-chi-bot bot commented May 21, 2024

*: use a separate runner for updating subtree #8158

*: use a separate runner for updating subtree #8158

Conversation

rleungx commented May 9, 2024 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented May 9, 2024 • edited

codecov bot commented May 9, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx commented May 21, 2024

ti-chi-bot bot commented May 21, 2024

ti-chi-bot bot commented May 21, 2024

rleungx commented May 9, 2024 •

edited

ti-chi-bot bot commented May 9, 2024 •

edited

codecov bot commented May 9, 2024 •

edited