mcs: make scheduling server support operator http interface #7090

lhy1024 · 2023-09-13T15:30:57Z

What problem does this PR solve?

Issue Number: Ref #5839

What is changed and how does it work?

It can be reviewed after #7113 and #7120

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2023-09-13T15:31:01Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

HuSharp
rleungx

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-09-13T15:31:01Z

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

Signed-off-by: lhy1024 <admin@liudos.us>

codecov · 2023-09-26T06:04:15Z

Codecov Report

Merging #7090 (5e4559f) into master (eb6953f) will increase coverage by 0.35%.
Report is 4 commits behind head on master.
The diff coverage is 63.70%.

❗ Current head 5e4559f differs from pull request most recent head b5e8d21. Consider uploading reports for the commit b5e8d21 to get more accurate results

@@            Coverage Diff             @@
##           master    #7090      +/-   ##
==========================================
+ Coverage   74.26%   74.62%   +0.35%     
==========================================
  Files         441      441              
  Lines       47216    47282      +66     
==========================================
+ Hits        35064    35283     +219     
+ Misses       9063     8913     -150     
+ Partials     3089     3086       -3

Flag	Coverage Δ
unittests	`74.62% <63.70%> (+0.35%)`	⬆️

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

rleungx · 2023-09-26T09:41:18Z

pkg/mcs/scheduling/server/apis/v1/api.go

+// @Produce  json
+// @Success  200  {object}  []operator.OpRecord
+// @Failure  400  {string}  string  "The request is invalid."
+// @Failure  500  {string}  string  "PD server failed to proceed the request."


Do we have it here?

It is a copy from server/api, maybe we need to fix all other comments too.

rleungx · 2023-09-26T09:42:05Z

pkg/mcs/scheduling/server/server.go

@@ -469,6 +469,12 @@ func (s *Server) startWatcher() (err error) {
 	return err
 }

+// GetOpts returns the persist config.
+// It's used to test.
+func (s *Server) GetOpts() *config.PersistConfig {


How about using getcluster?

It has not SetPlacementRuleEnabled

Maybe you can add it?

I will add it in another pr about refactor cluster

Or rename it to GetPersistConfig

server/api/operator.go

rleungx · 2023-09-26T09:46:07Z

pkg/utils/apiutil/apiutil.go

+// ParseTime parses a time string with the format "1694580288"
+func ParseTime(t string) (time.Time, error) {
+	if len(t) == 0 {
+		return time.Time{}, nil


Do we need to return an error?

No, for http api if there is no "from", it will return all results. I added a comment about it.

rleungx · 2023-09-26T09:47:39Z

tests/integrations/mcs/scheduling/api_test.go

@@ -132,10 +132,21 @@ func (suite *apiTestSuite) TestAPIForward() {
 	re.NoError(err)
 	re.Len(slice, 0)

-	err = testutil.ReadGetJSON(re, testDialClient, fmt.Sprintf("%s/%s", urlPrefix, "operators/2"), &resp,
-		testutil.WithHeader(re, apiutil.ForwardToMicroServiceHeader, "true"))
+	err = testutil.CheckPostJSON(testDialClient, fmt.Sprintf("%s/%s", urlPrefix, "operators"), []byte(``),


Do we need to add a successful get test?

This test only tests forward. And a successful get test will be tested in tests/server/api and tests/pdctl.

Why not also add it here?

tests/server/api/operator_test.go

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx · 2023-09-27T03:44:07Z

server/api/trend.go

+		from time.Time
+		err  error
+	)
+	if froms := r.URL.Query()["from"]; len(froms) > 0 {


Do we need to maintain this interface?

I am not sure, this pr only keep the same with pd mode.

tests/testutil.go

rleungx

The rest LGTM

Signed-off-by: lhy1024 <admin@liudos.us>

HuSharp · 2023-09-27T05:00:34Z

pkg/schedule/handler/handler.go

+	default:
+		return http.StatusBadRequest, nil, errors.Errorf("unknown operator")
+	}
+	return http.StatusOK, nil, errors.Errorf("The operator is created.")


Does return error here make sense?

HuSharp · 2023-09-27T05:05:46Z

pkg/mcs/scheduling/server/apis/v1/api.go

+	c.IndentedJSON(http.StatusOK, records)
+}
+
+// FIXME: details of input json body params


What does this mean? 👀

only copy from previous code

Signed-off-by: lhy1024 <admin@liudos.us>

HuSharp

LGTM!

lhy1024 · 2023-09-27T11:44:22Z

/merge

ti-chi-bot · 2023-09-27T11:44:23Z

@lhy1024: 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-27T11:44:25Z

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

Commit hash: 5e4559f

ti-chi-bot · 2023-09-27T11:44:38Z

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

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.

ref tikv#5839 Signed-off-by: lhy1024 <admin@liudos.us> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot bot requested review from HunDunDM and nolouch September 13, 2023 15:31

mcs: make scheduling server support operator http interface

c4c9359

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 force-pushed the sche-redirect2 branch from cfa1860 to c4c9359 Compare September 19, 2023 06:35

add some test

9aa69f1

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 force-pushed the sche-redirect2 branch from bc94273 to 9aa69f1 Compare September 19, 2023 09:42

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

lhy1024 added 7 commits September 20, 2023 14:40

Merge branch 'master' of github.com:tikv/pd into sche-redirect2

0dae55b

add test after region heartbeat forward

d8af115

Signed-off-by: lhy1024 <admin@liudos.us>

wait review

09ca45d

Signed-off-by: lhy1024 <admin@liudos.us>

move function out from pd-ctl

8f46243

Signed-off-by: lhy1024 <admin@liudos.us>

add more test

51780f2

Signed-off-by: lhy1024 <admin@liudos.us>

Merge branch 'master' of github.com:tikv/pd into sche-redirect2

27b4458

Signed-off-by: lhy1024 <admin@liudos.us>

fix conflict

7b9b756

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 marked this pull request as ready for review September 21, 2023 09:32

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

lhy1024 requested review from JmPotato and rleungx and removed request for nolouch and HunDunDM September 21, 2023 09:32

lhy1024 mentioned this pull request Sep 21, 2023

mcs: make scheduling server support checker and scheduler http interface #7131

Merged

make test stable

3a7d8b2

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx reviewed Sep 26, 2023

View reviewed changes

address comments

37b3c53

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx reviewed Sep 27, 2023

View reviewed changes

tests/testutil.go Show resolved Hide resolved

rleungx approved these changes Sep 27, 2023

View reviewed changes

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

rename

2fb4aa3

Signed-off-by: lhy1024 <admin@liudos.us>

HuSharp reviewed Sep 27, 2023

View reviewed changes

address comments

5e4559f

Signed-off-by: lhy1024 <admin@liudos.us>

HuSharp approved these changes Sep 27, 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 27, 2023

rleungx mentioned this pull request Sep 27, 2023

Separate scheduling into an independent service #5839

Closed

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

Merge branch 'master' into sche-redirect2

b5e8d21

ti-chi-bot bot merged commit 7a9e566 into tikv:master Sep 27, 2023
22 checks passed

lhy1024 mentioned this pull request Oct 9, 2023

mcs: forward request to scheduling server #7178

Closed

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs: make scheduling server support operator http interface #7090

mcs: make scheduling server support operator http interface #7090

lhy1024 commented Sep 13, 2023 •

edited

Loading

ti-chi-bot bot commented Sep 13, 2023 •

edited

Loading

ti-chi-bot bot commented Sep 13, 2023

codecov bot commented Sep 26, 2023 •

edited

Loading

rleungx Sep 26, 2023

lhy1024 Sep 26, 2023

rleungx Sep 26, 2023

lhy1024 Sep 26, 2023

rleungx Sep 27, 2023

lhy1024 Sep 27, 2023

rleungx Sep 27, 2023

rleungx Sep 26, 2023

lhy1024 Sep 26, 2023 •

edited

Loading

rleungx Sep 26, 2023

lhy1024 Sep 26, 2023

rleungx Sep 27, 2023

rleungx Sep 27, 2023

lhy1024 Sep 27, 2023

rleungx left a comment

HuSharp Sep 27, 2023

HuSharp Sep 27, 2023

lhy1024 Sep 27, 2023

HuSharp left a comment

lhy1024 commented Sep 27, 2023

ti-chi-bot bot commented Sep 27, 2023

ti-chi-bot bot commented Sep 27, 2023

ti-chi-bot bot commented Sep 27, 2023

mcs: make scheduling server support operator http interface #7090

mcs: make scheduling server support operator http interface #7090

Conversation

lhy1024 commented Sep 13, 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 Sep 13, 2023 • edited Loading

ti-chi-bot bot commented Sep 13, 2023

codecov bot commented Sep 26, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhy1024 Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuSharp left a comment

Choose a reason for hiding this comment

lhy1024 commented Sep 27, 2023

ti-chi-bot bot commented Sep 27, 2023

ti-chi-bot bot commented Sep 27, 2023

ti-chi-bot bot commented Sep 27, 2023

lhy1024 commented Sep 13, 2023 •

edited

Loading

ti-chi-bot bot commented Sep 13, 2023 •

edited

Loading

codecov bot commented Sep 26, 2023 •

edited

Loading

lhy1024 Sep 26, 2023 •

edited

Loading