*: create coordinator with scheduling cluster #6893

rleungx · 2023-08-03T10:08:18Z

What problem does this PR solve?

Issue Number: Ref #5839.

What is changed and how does it work?

It can be reviewed until #6891 is merged.

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2023-08-03T10:08:20Z

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

rleungx · 2023-08-03T10:27:40Z

/test build

codecov · 2023-08-03T10:35:08Z

Codecov Report

Merging #6893 (4401721) into master (38d087f) will decrease coverage by 0.13%.
The diff coverage is 30.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6893      +/-   ##
==========================================
- Coverage   74.39%   74.27%   -0.13%     
==========================================
  Files         427      427              
  Lines       45163    45224      +61     
==========================================
- Hits        33598    33589       -9     
- Misses       8600     8676      +76     
+ Partials     2965     2959       -6

Flag	Coverage Δ
unittests	`74.27% <30.92%> (-0.13%)`	⬇️

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

lhy1024

the rest LGTM

lhy1024 · 2023-08-07T12:00:06Z

pkg/schedule/splitter/region_splitter.go

 }

 // NewRegionSplitter return a region splitter
-func NewRegionSplitter(cluster sche.ClusterInformer, handler SplitRegionsHandler) *RegionSplitter {
+func NewRegionSplitter(cluster sche.ClusterInformer, handler SplitRegionsHandler, addSuspectRegions func(ids ...uint64)) *RegionSplitter {


Do we have other ways to avoid pass a function to it?

Any better idea?

lhy1024 · 2023-08-07T12:02:53Z

pkg/mcs/scheduling/server/config/config.go

-// GetMergeScheduleLimit returns the merge schedule limit.
-func (o *PersistConfig) GetMergeScheduleLimit() uint64 {
- return o.GetScheduleConfig().MergeScheduleLimit
+// GetMaxMergeRegionSize returns the max region size.


In future, we will need to add two in pkg/mcs/scheduling/server/config/config.go and server/config/persist_options.go?

JmPotato · 2023-08-08T09:04:26Z

pkg/mcs/scheduling/server/cluster.go

+func (c *Cluster) GetPersistOptions() sc.ConfProvider {
+ return c.persistConfig
+}


This is kind of weird to me since the method name is GetPersistOptions but returns an interface. Maybe we could keep it returning config.PersistOptions and give it to the caller to choose the specific acceptance type?

The function is part of the ClusterInformer interface.

And I remove the GetPersistOptions from that interface.

JmPotato

LGTM. Please resolve the conflicts.

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

rleungx · 2023-08-10T06:56:05Z

/merge

ti-chi-bot · 2023-08-10T06:56:06Z

@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-08-10T06:56:08Z

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

Commit hash: 4401721

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

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 3, 2023

ti-chi-bot bot requested review from HunDunDM and Yisaer August 3, 2023 10:08

rleungx requested review from JmPotato and lhy1024 and removed request for Yisaer and HunDunDM August 3, 2023 10:08

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

rleungx force-pushed the cluster branch from f7b184f to 6e36619 Compare August 7, 2023 07:20

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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 7, 2023

lhy1024 reviewed Aug 7, 2023

View reviewed changes

JmPotato reviewed Aug 8, 2023

View reviewed changes

rleungx requested review from JmPotato and lhy1024 August 8, 2023 10:06

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

JmPotato approved these changes Aug 10, 2023

View reviewed changes

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

rleungx added 4 commits August 10, 2023 14:00

merge hot stats

56e06d8

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

create coordinator with scheduling cluster

26626c9

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

remove GetPersistOptions for cluster

07ac843

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

resolve the conflicts

4401721

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

rleungx force-pushed the cluster branch from 619a97e to 4401721 Compare August 10, 2023 06:36

ti-chi-bot bot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 10, 2023

ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 10, 2023

lhy1024 approved these changes Aug 10, 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 Aug 10, 2023

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

ti-chi-bot bot merged commit af35e15 into tikv:master Aug 10, 2023
21 of 23 checks passed

rleungx mentioned this pull request Aug 24, 2023

Separate scheduling into an independent service #5839

Closed

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

*: create coordinator with scheduling cluster (tikv#6893)

19fbf4d

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: create coordinator with scheduling cluster #6893

*: create coordinator with scheduling cluster #6893

rleungx commented Aug 3, 2023 •

edited

Loading

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

edited

Loading

rleungx commented Aug 3, 2023

codecov bot commented Aug 3, 2023 •

edited

Loading

lhy1024 left a comment

lhy1024 Aug 7, 2023

rleungx Aug 8, 2023

lhy1024 Aug 7, 2023

rleungx Aug 8, 2023

JmPotato Aug 8, 2023

rleungx Aug 8, 2023

rleungx Aug 8, 2023

JmPotato left a comment •

edited

Loading

rleungx commented Aug 10, 2023

ti-chi-bot bot commented Aug 10, 2023

ti-chi-bot bot commented Aug 10, 2023

*: create coordinator with scheduling cluster #6893

*: create coordinator with scheduling cluster #6893

Conversation

rleungx commented Aug 3, 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 3, 2023 • edited Loading

rleungx commented Aug 3, 2023

codecov bot commented Aug 3, 2023 • edited Loading

Codecov Report

lhy1024 left a comment

Choose a reason for hiding this comment

lhy1024 Aug 7, 2023

Choose a reason for hiding this comment

rleungx Aug 8, 2023

Choose a reason for hiding this comment

lhy1024 Aug 7, 2023

Choose a reason for hiding this comment

rleungx Aug 8, 2023

Choose a reason for hiding this comment

JmPotato Aug 8, 2023

Choose a reason for hiding this comment

rleungx Aug 8, 2023

Choose a reason for hiding this comment

rleungx Aug 8, 2023

Choose a reason for hiding this comment

JmPotato left a comment • edited Loading

Choose a reason for hiding this comment

rleungx commented Aug 10, 2023

ti-chi-bot bot commented Aug 10, 2023

ti-chi-bot bot commented Aug 10, 2023

rleungx commented Aug 3, 2023 •

edited

Loading

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

edited

Loading

codecov bot commented Aug 3, 2023 •

edited

Loading

JmPotato left a comment •

edited

Loading