resource_manager: ignore ru metrics for background request by HuSharp · Pull Request #6766 · tikv/pd

HuSharp · 2023-07-06T01:32:07Z

What problem does this PR solve?

Issue Number: Ref #5851

What is changed and how does it work?

Check List

Tests

Integration test
Manual test (add detailed scripts or steps below)

Release note

None.

ti-chi-bot · 2023-07-06T01:32:09Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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.

Details

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

ti-chi-bot · 2023-07-06T01:32:09Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-07-06T08:52:37Z

Codecov Report

Merging #6766 (9e01e14) into master (4d9455d) will decrease coverage by 0.11%.
The diff coverage is 74.28%.

❗ Current head 9e01e14 differs from pull request most recent head 295a382. Consider uploading reports for the commit 295a382 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6766      +/-   ##
==========================================
- Coverage   74.32%   74.22%   -0.11%     
==========================================
  Files         414      414              
  Lines       43447    43469      +22     
==========================================
- Hits        32294    32264      -30     
- Misses       8297     8337      +40     
- Partials     2856     2868      +12

Flag	Coverage Δ
unittests	`74.22% <74.28%> (-0.11%)`	⬇️

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

client/resource_group/controller/controller.go

nolouch · 2023-07-17T03:04:42Z

pkg/mcs/resourcemanager/server/metrics.go

 		}, []string{resourceGroupNameLabel})
+
+	// Background RU cost metrics.
+	backgroundReadRequestUnitCost = prometheus.NewCounterVec(


maybe add a type as we disscussed?

glorv · 2023-07-17T07:23:57Z

client/resource_group/controller/controller.go

+		return false
+	}
+
+	gc.metaLock.Lock()


Is Rwlock better here as this call can in the hot path

glorv · 2023-07-17T07:24:22Z

client/resource_group/controller/controller.go

+		if len(requestResource) == 0 || len(bg.JobTypes) == 0 {
+			return false
+		}
+		if idx := strings.LastIndex(requestResource, "_"); idx != -1 {


Should also implement the fallback to default logic here

https://github.com/tikv/tikv/pull/15100/files#diff-4fc0c7882a408c1e08b80a597c1d551dd92301f7f6da911b5993e41ac82a265eR244 You mean just like that?

Yes. That means if a non-default resource group's config is not explicitly set, you should uses the default group's rule to check it.

Signed-off-by: husharp <jinhao.hu@pingcap.com>

client/resource_group/controller/controller.go

Signed-off-by: husharp <jinhao.hu@pingcap.com>

nolouch

lgtm, plz update the proto.

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp · 2023-07-24T03:12:08Z

@glorv @CabinfeverB friendly ping~

glorv

LGTM

nolouch · 2023-07-24T07:54:45Z

/merge

ti-chi-bot · 2023-07-24T07:54:47Z

@nolouch: 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.

Details

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 · 2023-07-24T07:54:48Z

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

Details

Commit hash: 9e01e14

ti-chi-bot · 2023-07-24T07:55:01Z

@HuSharp: Your PR was out of date, I have automatically updated it for you.

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.

Details

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 bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 6, 2023

ti-chi-bot bot requested review from lhy1024 and rleungx July 6, 2023 01:32

ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 6, 2023

HuSharp requested review from CabinfeverB and nolouch and removed request for lhy1024 and rleungx July 6, 2023 01:32

HuSharp force-pushed the ignore_ru_for_background branch 5 times, most recently from 9ee0467 to b3a746e Compare July 6, 2023 08:05

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/needs-linked-issue labels Jul 6, 2023

HuSharp force-pushed the ignore_ru_for_background branch from b3a746e to 1015d67 Compare July 6, 2023 08:39

HuSharp marked this pull request as ready for review July 6, 2023 08:40

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2023

CabinfeverB reviewed Jul 6, 2023

View reviewed changes

client/resource_group/controller/controller.go Outdated Show resolved Hide resolved

client/resource_group/controller/controller.go Outdated Show resolved Hide resolved

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 6, 2023

HuSharp force-pushed the ignore_ru_for_background branch 2 times, most recently from 6b3ae84 to 5a34eab Compare July 7, 2023 06:35

HuSharp mentioned this pull request Jul 7, 2023

resource_manager: ignore ru metrics for background request tikv/client-go#872

Merged

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2023

nolouch reviewed Jul 17, 2023

View reviewed changes

HuSharp force-pushed the ignore_ru_for_background branch from d540c43 to 5ab1349 Compare July 17, 2023 07:19

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 17, 2023

glorv reviewed Jul 17, 2023

View reviewed changes

update test

78966d8

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the ignore_ru_for_background branch from 5ab1349 to 78966d8 Compare July 17, 2023 07:49

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 17, 2023

HuSharp force-pushed the ignore_ru_for_background branch from a88a8b6 to dfe5ede Compare July 19, 2023 01:44

add fallback

6698aa7

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the ignore_ru_for_background branch from dfe5ede to 6698aa7 Compare July 19, 2023 01:51

glorv reviewed Jul 19, 2023

View reviewed changes

client/resource_group/controller/controller.go Outdated Show resolved Hide resolved

HuSharp added 2 commits July 19, 2023 11:18

add getBackgroundSettings and add test

a233a44

Signed-off-by: husharp <jinhao.hu@pingcap.com>

use getMeta instead

4792714

Signed-off-by: husharp <jinhao.hu@pingcap.com>

nolouch approved these changes Jul 20, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 20, 2023

update kvproto

9e01e14

Signed-off-by: husharp <jinhao.hu@pingcap.com>

glorv reviewed Jul 24, 2023

View reviewed changes

nolouch added the require-LGT1 Indicates that the PR requires an LGTM. label Jul 24, 2023

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 24, 2023

Merge branch 'master' into ignore_ru_for_background

295a382

ti-chi-bot bot merged commit de985b8 into tikv:master Jul 24, 2023

HuSharp deleted the ignore_ru_for_background branch August 3, 2023 00:57

Conversation

HuSharp commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-chi-bot bot commented Jul 6, 2023

Uh oh!

codecov bot commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nolouch Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

glorv Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

glorv Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

HuSharp Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

glorv Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nolouch left a comment

Choose a reason for hiding this comment

Uh oh!

HuSharp commented Jul 24, 2023

Uh oh!

glorv left a comment

Choose a reason for hiding this comment

Uh oh!

nolouch commented Jul 24, 2023

Uh oh!

ti-chi-bot bot commented Jul 24, 2023

Uh oh!

ti-chi-bot bot commented Jul 24, 2023

Uh oh!

ti-chi-bot bot commented Jul 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HuSharp commented Jul 6, 2023 •

edited

Loading

ti-chi-bot bot commented Jul 6, 2023 •

edited

Loading

codecov bot commented Jul 6, 2023 •

edited

Loading