Skip to content

resource_metering: introduce pub/sub mode#11087

Closed
zhongzc wants to merge 21 commits intotikv:masterfrom
zhongzc:pubsub
Closed

resource_metering: introduce pub/sub mode#11087
zhongzc wants to merge 21 commits intotikv:masterfrom
zhongzc:pubsub

Conversation

@zhongzc
Copy link
Contributor

@zhongzc zhongzc commented Oct 18, 2021 •
edited
Loading

Signed-off-by: Zhenchi zhongzc_arch@outlook.com

Make the diagnostic node dependent on TiKV instead of TiKV dependent on the diagnostic node to reverse the dependency to a normal form.

Depends on: pingcap/kvproto#827
Related: pingcap/tidb#29020

What is changed and how it works?

  • Deprecate config resource-metering.enabled
    • If exists external subscribers, the recorder will be enabled. If no external subscribers, the recorder will be paused. The config resource-metering.enabled takes no effect anymore.
  • Provide gRPC service kvproto::resource_usage_agent_grpc::ResourceMeteringPubSub
    • Clients can subscribe to resource metering records through this service.
  • Modify trait resource_metering::Client
    • Add methods is_pending and is_closed. Reporter depends on them to enable/disable the recorder.
    • Add a new implementation PubClient for connecting the reporter and external subscribers.

Check List

Tests

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

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM

Release note 

Add resource metering pub/sub gRPC service for publishing records

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 18, 2021

[REVIEW NOTIFICATION]

This pull request has not been approved.

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 ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/invalid-commit-message size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 18, 2021
zhongzc added 10 commits October 25, 2021 14:21
@zhongzc
resource_metering: introduce pub/sub mode
d00cbbf 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
fix
6862e10 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
add integration tests
e435836 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
fix tests
807c497 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
tidy
fcd9851 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
fix blocking
056e4c3 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
enabled by default
74ea9c3 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
deprecate the enabled config
4cf2a89 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
tidy
0d1928d 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
tmp
ddc3df3 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
fix compiling
12565d7 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
refactor
fec71ca 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
fix integration tests
abb2274 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc zhongzc marked this pull request as ready for review October 26, 2021 08:42
@ti-chi-bot 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 Oct 26, 2021
@zhongzc zhongzc mentioned this pull request Oct 26, 2021
Closed
5 tasks
@zhongzc
Copy link
Contributor Author

zhongzc commented Oct 26, 2021

/cc @crazycs520 @mornyx @breeswish

@ti-chi-bot ti-chi-bot requested a review from breezewish October 26, 2021 09:05
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 26, 2021

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

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

Details

In response to this:

/cc @crazycs520 @mornyx @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.

@zhongzc
add metrics
eb23dd3 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
add metrics
ef779cb 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
is_closed -> is_down
9224a9f 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2021
@zhongzc
Merge remote-tracking branch 'origin/master' into pubsub
69de6a7
@zhongzc
refactor
eab779a 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2021
@andylokandy
Copy link
Contributor

andylokandy commented Nov 22, 2021 •
edited
Loading

/cc @andylokandy

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2021
@zhongzc
merge origin/master
b0705b0 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2021
@zhongzc
polish
9822691 
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 6, 2021

[FORMAT CHECKER NOTIFICATION]

Please provide the associated issue number in the commit messages, for example: close #12345, or ref #12345.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 9, 2021

@zhongzc: PR needs rebase.

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

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2021
@breezewish
Copy link
Member

breezewish commented Dec 14, 2021 •
edited
Loading

Maybe we can hold the review and the update of this PR until the change of Pub/Sub at TiDB side comes to an agreement since mostly they are similar designs.

@zhongzc zhongzc mentioned this pull request Dec 21, 2021
Merged
@zhongzc zhongzc closed this Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@breezewish breezewish Awaiting requested review from breezewish

@andylokandy andylokandy Awaiting requested review from andylokandy

Assignees

No one assigned

Labels

do-not-merge/invalid-commit-message needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zhongzc @ti-chi-bot @andylokandy @breezewish