resrc_mtr: reorganize code structure for flexibility by mornyx · Pull Request #10998 · tikv/tikv

mornyx · 2021-09-28T07:59:32Z

What problem does this PR solve?

Reorganize resource-metering code structure to facilitate the addition of new modules.

What's Changed:

Promote most of the future reusable modules to the root path.
Split the Recorder into a SubRecorder trait for the collection of various resources.

Related changes

PR to update pingcap/kvproto#814

Check List

Tests

Unit test
Integration test

Release note

None

…n of new modules Signed-off-by: mornyx <mornyx.z@gmail.com>

ti-chi-bot · 2021-09-28T07:59:35Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

andylokandy
zhongzc

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.

Signed-off-by: mornyx <mornyx.z@gmail.com>

mornyx · 2021-09-28T10:47:57Z

This PR was separated from tikv#10973 and only changes the code structure without adding the summary module. PTAL, Thanks!

/cc @zhongzc @crazycs520 @breeswish

ti-chi-bot · 2021-09-28T10:48:00Z

@mornyx: GitHub didn't allow me to request PR reviews from the following users: crazycs520.

Note that only tikv members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

This PR is separated from tikv#10973 and only changes the code structure without adding the summary module. PTAL, Thanks!

/cc @zhongzc @crazycs520 @breeswish

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 kubernetes/test-infra repository.

components/resource_metering/src/recorder/mod.rs

Cargo.toml

zhongzc · 2021-09-30T09:48:04Z

tests/integrations/resource_metering/test_dynamic_config.rs

+    // TODO(mornyx): Review this case.
+    //               (According to my personal observation, this should not be empty.)
+    //
+    // assert!(test_suite.fetch_reported_cpu_time().is_empty());


The logic here comes from the fact that if the reporter's address was not set, the corresponding collector would be unregistered and the recorder would hang due to no collector. The recorder would restart on L35, but the result would be empty because the workload was cancelled in time on L31.

Nowadays collectors are not only the ones corresponding to the reporter but also the ones registered by the pd statistics collector, so the recorder keeps running. I think we can add a line between L35 and L36

test_suite.flush_agent();

to fix it.

👍
The reason for this problem is that I changed the Collector in Recorder to static binding.

I tried this method on the latest code, but the integration test still failed (pipeline here).

I think if we do clean up first and then wait, we will still receive the data, so I put test_suite.flush_receiver() between L36 and L37, and it passed the test. PLTA: test_dynamic_config.rs#L37.

Signed-off-by: mornyx <mornyx.z@gmail.com>

crazycs520 · 2021-10-12T10:18:10Z

components/resource_metering/Cargo.toml

 futures = "0.3"
 pdqselect = "0.1"
 fail = "0.4"
+thread-id = "4.0.0"


It is never used?

According to utils.rs#L31，we also provide a way to get the thread id on non-linux systems. It is only used to distinguish different threads, not for actual CPU sampling work.

crazycs520

LGTM

ti-chi-bot · 2021-10-12T10:20:30Z

@crazycs520: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

Details

In response to this:

LGTM

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.

Signed-off-by: mornyx <mornyx.z@gmail.com>

components/resource_metering/src/client.rs

zhongzc · 2021-10-14T10:35:55Z

LGTM

Co-authored-by: Zhenchi <zhongzc_arch@outlook.com> Signed-off-by: mornyx <mornyx.z@gmail.com>

Signed-off-by: mornyx <mornyx.z@gmail.com>

zhongzc · 2021-10-15T08:38:46Z

/merge

ti-chi-bot · 2021-10-15T08:38:47Z

@zhongzc: 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 · 2021-10-15T08:38:49Z

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

Details

Commit hash: 0d54eb4

ti-chi-bot · 2021-10-15T09:01:37Z

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

At the same time I will also trigger all tests for you:

/run-all-tests

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.

zhongzc · 2021-10-15T10:27:05Z

/test

Reorganize resource-metering code structure to facilitate the additio…

0b397aa

…n of new modules Signed-off-by: mornyx <mornyx.z@gmail.com>

ti-chi-bot added 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 Sep 28, 2021

ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Sep 28, 2021

ti-chi-bot added contribution This PR is from a community contributor. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 28, 2021

mornyx added 3 commits September 28, 2021 16:00

Merge branch 'master' into resource-metering-framework

8072120

Fix the inconsistency of clock_t between different systems

3894a64

Signed-off-by: mornyx <mornyx.z@gmail.com>

Fix recorder_test

daa3499

Signed-off-by: mornyx <mornyx.z@gmail.com>

mornyx force-pushed the resource-metering-framework branch from 4c5ca18 to daa3499 Compare September 28, 2021 08:33

mornyx added 4 commits September 28, 2021 17:51

Remove unnecessary tests

6392440

Signed-off-by: mornyx <mornyx.z@gmail.com>

Fix typo

bccd1f1

Signed-off-by: mornyx <mornyx.z@gmail.com>

Re-group imports

117c696

Signed-off-by: mornyx <mornyx.z@gmail.com>

Fix test_collect

473bad3

Signed-off-by: mornyx <mornyx.z@gmail.com>

ti-chi-bot requested review from breezewish and zhongzc September 28, 2021 10:47

mornyx marked this pull request as ready for review September 28, 2021 10:48

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

zhongzc reviewed Sep 30, 2021

View reviewed changes

components/resource_metering/src/recorder/mod.rs Outdated Show resolved Hide resolved

zhongzc reviewed Sep 30, 2021

View reviewed changes

components/resource_metering/src/recorder/mod.rs Outdated Show resolved Hide resolved

zhongzc reviewed Sep 30, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

zhongzc reviewed Sep 30, 2021

View reviewed changes

Add enable param for Recorder; Fix integrations

44c7a1c

Signed-off-by: mornyx <mornyx.z@gmail.com>

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2021

Merge branch 'master' into resource-metering-framework

2183752

Signed-off-by: mornyx <mornyx.z@gmail.com>

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

Remove static binding of the Collector

befac98

Signed-off-by: mornyx <mornyx.z@gmail.com>

Merge branch 'master' into resource-metering-framework

6ec6429

crazycs520 reviewed Oct 12, 2021

View reviewed changes

crazycs520 approved these changes Oct 12, 2021

View reviewed changes

mornyx added 4 commits October 14, 2021 12:25

Merge branch 'master' into resource-metering-framework

93cda59

Try fix integrations

87c18d3

Signed-off-by: mornyx <mornyx.z@gmail.com>

Rename

bee51e3

Signed-off-by: mornyx <mornyx.z@gmail.com>

Adjust logic order

1b30610

Signed-off-by: mornyx <mornyx.z@gmail.com>

zhongzc reviewed Oct 14, 2021

View reviewed changes

components/resource_metering/src/client.rs Outdated Show resolved Hide resolved

Update components/resource_metering/src/client.rs

324df63

Co-authored-by: Zhenchi <zhongzc_arch@outlook.com> Signed-off-by: mornyx <mornyx.z@gmail.com>

mornyx force-pushed the resource-metering-framework branch from d1fce5f to 324df63 Compare October 15, 2021 04:37

fmt

0d54eb4

Signed-off-by: mornyx <mornyx.z@gmail.com>

zhongzc approved these changes Oct 15, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 15, 2021

andylokandy approved these changes Oct 15, 2021

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 Oct 15, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 15, 2021

ti-chi-bot added 2 commits October 15, 2021 16:54

Merge branch 'master' into resource-metering-framework

111c1aa

Merge branch 'master' into resource-metering-framework

4f1ecca

ti-chi-bot merged commit b452087 into tikv:master Oct 15, 2021

This was referenced Oct 18, 2021

resrc_mtr: add summary mod with a common framework #10973

Closed

resrc_mtr: introduce scanned keys statistics #11085

Merged

mornyx deleted the resource-metering-framework branch December 10, 2021 08:18

Conversation

mornyx commented Sep 28, 2021

What problem does this PR solve?

Related changes

Check List

Release note

Uh oh!

ti-chi-bot commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mornyx commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-chi-bot commented Sep 28, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhongzc Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mornyx Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

zhongzc Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

mornyx Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

crazycs520 Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

mornyx Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

crazycs520 left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot commented Oct 12, 2021

Uh oh!

Uh oh!

zhongzc commented Oct 14, 2021

Uh oh!

zhongzc commented Oct 15, 2021

Uh oh!

ti-chi-bot commented Oct 15, 2021

Uh oh!

ti-chi-bot commented Oct 15, 2021

Uh oh!

ti-chi-bot commented Oct 15, 2021

Uh oh!

zhongzc commented Oct 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ti-chi-bot commented Sep 28, 2021 •

edited

Loading

mornyx commented Sep 28, 2021 •

edited

Loading

zhongzc Sep 30, 2021 •

edited

Loading