Skip to content

Support resource group watch#5830

Merged
ti-chi-bot merged 12 commits intotikv:masterfrom
HuSharp:support_resource_group_watch
Jan 19, 2023
Merged

Support resource group watch#5830
ti-chi-bot merged 12 commits intotikv:masterfrom
HuSharp:support_resource_group_watch

Conversation

@HuSharp
Copy link
Member

@HuSharp HuSharp commented Jan 5, 2023 •
edited
Loading

What problem does this PR solve?

Issue Number: Close #5794 Ref tikv/tikv#14022

What is changed and how does it work?

Support resource group watch

Check List

Tests

  • Unit test
  • Integration test

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jan 5, 2023 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • CabinfeverB
  • 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 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. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Jan 5, 2023
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jan 5, 2023

Hi @HuSharp. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 requested review from HunDunDM and rleungx January 5, 2023 01:34
@HuSharp HuSharp force-pushed the support_resource_group_watch branch from 3fc7577 to 4366236 Compare January 5, 2023 01:45
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2023
@HuSharp HuSharp force-pushed the support_resource_group_watch branch 5 times, most recently from fb9a3db to 23c074a Compare January 10, 2023 10:12
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2023
@HuSharp HuSharp force-pushed the support_resource_group_watch branch from 003db29 to 51f33da Compare January 11, 2023 02:52
@CabinfeverB CabinfeverB mentioned this pull request Jan 11, 2023
Closed
7 tasks
@HuSharp HuSharp force-pushed the support_resource_group_watch branch from 51f33da to 4b06244 Compare January 12, 2023 02:59
@CabinfeverB CabinfeverB self-requested a review January 12, 2023 03:03
CabinfeverB
CabinfeverB reviewed Jan 12, 2023
View reviewed changes
@CabinfeverB CabinfeverB requested review from nolouch and removed request for HunDunDM and rleungx January 12, 2023 07:10
@HuSharp HuSharp force-pushed the support_resource_group_watch branch 2 times, most recently from 9cfe759 to 492c62c Compare January 12, 2023 10:44
@HuSharp HuSharp marked this pull request as ready for review January 12, 2023 10:59
@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 Jan 12, 2023
@codecov
Copy link

codecov bot commented Jan 12, 2023 •
edited
Loading

Codecov Report

Base: 75.42% // Head: 75.46% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (639121e) compared to base (7ca1b9a).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5830      +/-   ##
==========================================
+ Coverage   75.42%   75.46%   +0.03%     
==========================================
  Files         341      341              
  Lines       34336    34404      +68     
==========================================
+ Hits        25897    25962      +65     
+ Misses       6209     6196      -13     
- Partials     2230     2246      +16
Flag Coverage Δ
unittests 75.46% <66.66%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
server/grpc_service.go 49.25% <53.84%> (+1.69%) ⬆️
client/resourcemanager_client.go 65.83% <68.96%> (+0.16%) ⬆️
client/client.go 70.89% <72.97%> (+0.34%) ⬆️
server/server.go 75.03% <100.00%> (+0.03%) ⬆️
server/api/diagnostic.go 76.92% <0.00%> (-23.08%) ⬇️
pkg/storage/endpoint/rule.go 83.33% <0.00%> (-11.12%) ⬇️
...erver/config/service_middleware_persist_options.go 91.66% <0.00%> (-8.34%) ⬇️
pkg/dashboard/adapter/manager.go 74.71% <0.00%> (-6.90%) ⬇️
server/cluster/diagnostic_manager.go 72.04% <0.00%> (-4.31%) ⬇️
pkg/storage/kv/etcd_kv.go 72.22% <0.00%> (-3.18%) ⬇️
... and 27 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.

@rleungx rleungx mentioned this pull request Jan 13, 2023
Merged
@HuSharp
support etcd and resource group
3d5218d 
Signed-off-by: husharp <jinhao.hu@pingcap.com>
@HuSharp
Copy link
Member Author

HuSharp commented Jan 18, 2023

image

Is lower code coverage a blocker issue ?

This does not block merge

glorv
glorv reviewed Jan 18, 2023
View reviewed changes
client/client.go
// TODO: complete this function with new implementation.
return nil, nil
// register watch components
globalConfigWatcherCh := make(chan []GlobalConfigItem, 16)
Copy link
Contributor

@glorv glorv Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to return the max revision of each []GlobalConfigItem so the caller can do incremental retry?

Copy link
Member Author

@HuSharp HuSharp Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since resource client is just for testing, I will add a retry mechanism after I solve pd client/tidb/CDC compatibility...

glorv
glorv reviewed Jan 19, 2023
View reviewed changes
@HuSharp
update kvproto and address comment
f7c1912 
Signed-off-by: husharp <jinhao.hu@pingcap.com>
@HuSharp
Copy link
Member Author

HuSharp commented Jan 19, 2023

@glorv @nolouch @BornChanger @CabinfeverB pls take a look, thx!

@HuSharp
make format happy
eca0d95 
Signed-off-by: husharp <jinhao.hu@pingcap.com>
glorv
glorv approved these changes Jan 19, 2023
View reviewed changes
Copy link
Contributor

@glorv glorv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jan 19, 2023

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

@HuSharp
add some comment
4bd95b5 
Signed-off-by: husharp <jinhao.hu@pingcap.com>
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 19, 2023
@ti-chi-bot 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 Jan 19, 2023
@nolouch
Copy link
Contributor

nolouch commented Jan 19, 2023

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jan 19, 2023

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

/run-all-tests

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
Copy link
Member

ti-chi-bot commented Jan 19, 2023

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

DetailsCommit hash: 4bd95b5

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 19, 2023
@HuSharp
Copy link
Member Author

HuSharp commented Jan 19, 2023

/run-all-tests

@nolouch
Copy link
Contributor

nolouch commented Jan 19, 2023

/rebuild

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jan 19, 2023

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

@purelind
Copy link
Contributor

purelind commented Jan 19, 2023

/test build

@ti-chi-bot ti-chi-bot merged commit c5bf5be into tikv:master Jan 19, 2023
@HuSharp HuSharp deleted the support_resource_group_watch branch January 19, 2023 09:50
@HuSharp HuSharp restored the support_resource_group_watch branch January 19, 2023 09:50
@HuSharp HuSharp deleted the support_resource_group_watch branch January 19, 2023 09:54
@overvenus overvenus mentioned this pull request Jan 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nolouch nolouch nolouch approved these changes

@CabinfeverB CabinfeverB CabinfeverB approved these changes

+1 more reviewer

@glorv glorv glorv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. release-note-none Denotes a PR that doesn't merit a release note. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Watch the resource group changes

7 participants

@HuSharp @ti-chi-bot @BornChanger @nolouch @purelind @glorv @CabinfeverB