mcs: forward store requests to scheduling server #6976

rleungx · 2023-08-23T07:49:37Z

What problem does this PR solve?

Issue Number: Ref #5839.

What is changed and how does it work?

This PR implements the basic functionality for synchronizing the store info.
Need to merge pingcap/kvproto#1183 first.

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2023-08-23T07:49:39Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
lhy1024

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.

ti-chi-bot · 2023-08-23T07:49:39Z

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

JmPotato · 2023-08-31T04:57:16Z

pkg/mcs/scheduling/server/grpc_service.go

+		// ignore the error
+		return &pdpb.StoreHeartbeatResponse{}, nil
+	}
+
+	// ignore the error


Maybe we should have a counter to record the error here.

add TODO here

JmPotato · 2023-08-31T04:59:00Z

server/grpc_service.go

@@ -979,6 +998,18 @@ func (s *GrpcServer) StoreHeartbeat(ctx context.Context, request *pdpb.StoreHear

 		s.handleDamagedStore(request.GetStats())

+		if s.IsAPIServiceMode() {


Is it possible to extract this logic to use a common function so that we could reuse it at line 862 as well?

codecov · 2023-09-06T09:54:05Z

Codecov Report

Merging #6976 (9783afc) into master (11b752e) will decrease coverage by 0.11%.
The diff coverage is 44.00%.

❗ Current head 9783afc differs from pull request most recent head 22945ac. Consider uploading reports for the commit 22945ac to get more accurate results

@@            Coverage Diff             @@
##           master    #6976      +/-   ##
==========================================
- Coverage   74.45%   74.35%   -0.11%     
==========================================
  Files         437      437              
  Lines       46533    46612      +79     
==========================================
+ Hits        34648    34659      +11     
- Misses       8840     8901      +61     
- Partials     3045     3052       +7

Flag	Coverage Δ
unittests	`74.35% <44.00%> (-0.11%)`	⬇️

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

JmPotato · 2023-09-13T09:25:34Z

server/grpc_service.go

+					},
+					Stats: request.GetStats(),
+				}
+				schedulingpb.NewSchedulingClient(client).StoreHeartbeat(ctx, req)


Is it possible to reuse the same client rather than create one each time?

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

lhy1024

the rest LGTM

lhy1024 · 2023-09-13T16:25:12Z

pkg/mcs/scheduling/server/cluster.go

+func (c *Cluster) getStoresWithoutLabelLocked(region *core.RegionInfo, key, value string) []*core.StoreInfo {
+	stores := make([]*core.StoreInfo, 0, len(region.GetPeers()))
+	for _, p := range region.GetPeers() {
+		if store := c.GetStore(p.StoreId); store != nil && !core.IsStoreContainLabel(store.GetMeta(), key, value) {


Suggested change

if store := c.GetStore(p.StoreId); store != nil && !core.IsStoreContainLabel(store.GetMeta(), key, value) {

if store := c.GetStore(p.GetStoreId()); store != nil && !core.IsStoreContainLabel(store.GetMeta(), key, value) {

lhy1024 · 2023-09-13T16:33:05Z

pkg/mcs/scheduling/server/cluster.go

+}
+
+// HandleStoreHeartbeat updates the store status.
+func (c *Cluster) HandleStoreHeartbeat(heartbeat *schedulingpb.StoreHeartbeatRequest) error {


It is too similar to the original HandleStoreHeartbeat, we can add some abstract functions for them?

good idea, I will do it in the next PR.

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

rleungx · 2023-09-14T03:25:18Z

/merge

ti-chi-bot · 2023-09-14T03:25:19Z

@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 · 2023-09-14T03:25:21Z

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

Commit hash: 22945ac

ref tikv#5839 Signed-off-by: Ryan Leung <rleungx@gmail.com>

ti-chi-bot bot requested review from JmPotato and nolouch August 23, 2023 07:49

rleungx requested review from lhy1024 and removed request for nolouch August 23, 2023 07:56

rleungx marked this pull request as ready for review August 24, 2023 08:20

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 Aug 24, 2023

rleungx force-pushed the forward-store branch from 8cfcb35 to 5f91e06 Compare August 30, 2023 10:09

JmPotato reviewed Aug 31, 2023

View reviewed changes

rleungx force-pushed the forward-store branch from 5f91e06 to 4e0c0e1 Compare September 1, 2023 07:13

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 1, 2023

rleungx force-pushed the forward-store branch 2 times, most recently from 200a944 to 9a30a83 Compare September 6, 2023 09:38

rleungx force-pushed the forward-store branch 2 times, most recently from 97e5d21 to 0579b2a Compare September 7, 2023 04:12

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

rleungx requested a review from JmPotato September 7, 2023 05:46

rleungx force-pushed the forward-store branch from 958aa0d to 30892d0 Compare September 7, 2023 06:06

rleungx mentioned this pull request Sep 7, 2023

Support forwarding store request pingcap/kvproto#1183

Merged

ti-chi-bot bot removed the do-not-merge/needs-linked-issue label Sep 7, 2023

rleungx mentioned this pull request Sep 7, 2023

Separate scheduling into an independent service #5839

Closed

rleungx force-pushed the forward-store branch from a2d01bd to d47f5d2 Compare September 7, 2023 09:01

JmPotato reviewed Sep 13, 2023

View reviewed changes

rleungx added 9 commits September 13, 2023 17:47

forward store requests to scheduling server

9e857be

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

tidy

2795df5

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

address the comment

4c51900

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

update kvproto

45aaf16

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

update kvproto

9996d5b

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

update kvproto

eff4122

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

fix ci

26dc6cf

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

remove PutStore

9da9146

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

update kvproto

53f609f

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

rleungx force-pushed the forward-store branch from 9783afc to d9a6c94 Compare September 13, 2023 10:03

rleungx requested a review from JmPotato September 13, 2023 10:03

address the comment

907ebd2

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

rleungx force-pushed the forward-store branch from d9a6c94 to 907ebd2 Compare September 13, 2023 10:17

lhy1024 reviewed Sep 13, 2023

View reviewed changes

address the comment

4fc1586

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

JmPotato approved these changes Sep 14, 2023

View reviewed changes

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

lhy1024 approved these changes Sep 14, 2023

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 Sep 14, 2023

rleungx added 2 commits September 14, 2023 11:20

address the comment

97fb042

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

Merge branch 'master' into forward-store

22945ac

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

ti-chi-bot bot merged commit 1a0c8fb into tikv:master Sep 14, 2023

rleungx deleted the forward-store branch September 14, 2023 03:37

rleungx added a commit to rleungx/pd that referenced this pull request Dec 1, 2023

mcs: forward store requests to scheduling server (tikv#6976)

7697108

ref tikv#5839 Signed-off-by: Ryan Leung <rleungx@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs: forward store requests to scheduling server #6976

mcs: forward store requests to scheduling server #6976

rleungx commented Aug 23, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 23, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 23, 2023

JmPotato Aug 31, 2023

rleungx Sep 7, 2023

JmPotato Aug 31, 2023

codecov bot commented Sep 6, 2023 •

edited

Loading

JmPotato Sep 13, 2023

lhy1024 left a comment

lhy1024 Sep 13, 2023

lhy1024 Sep 13, 2023

rleungx Sep 14, 2023

rleungx commented Sep 14, 2023

ti-chi-bot bot commented Sep 14, 2023

ti-chi-bot bot commented Sep 14, 2023

		@@ -979,6 +998,18 @@ func (s GrpcServer) StoreHeartbeat(ctx context.Context, request pdpb.StoreHear

		s.handleDamagedStore(request.GetStats())

		if s.IsAPIServiceMode() {

	if store := c.GetStore(p.StoreId); store != nil && !core.IsStoreContainLabel(store.GetMeta(), key, value) {
	if store := c.GetStore(p.GetStoreId()); store != nil && !core.IsStoreContainLabel(store.GetMeta(), key, value) {

mcs: forward store requests to scheduling server #6976

mcs: forward store requests to scheduling server #6976

Conversation

rleungx commented Aug 23, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Aug 23, 2023 • edited Loading

ti-chi-bot bot commented Aug 23, 2023

JmPotato Aug 31, 2023

Choose a reason for hiding this comment

rleungx Sep 7, 2023

Choose a reason for hiding this comment

JmPotato Aug 31, 2023

Choose a reason for hiding this comment

codecov bot commented Sep 6, 2023 • edited Loading

Codecov Report

JmPotato Sep 13, 2023

Choose a reason for hiding this comment

lhy1024 left a comment

Choose a reason for hiding this comment

lhy1024 Sep 13, 2023

Choose a reason for hiding this comment

lhy1024 Sep 13, 2023

Choose a reason for hiding this comment

rleungx Sep 14, 2023

Choose a reason for hiding this comment

rleungx commented Sep 14, 2023

ti-chi-bot bot commented Sep 14, 2023

ti-chi-bot bot commented Sep 14, 2023

rleungx commented Aug 23, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 23, 2023 •

edited

Loading

codecov bot commented Sep 6, 2023 •

edited

Loading