Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scheduler: add new api to add a new job for the special scheduler #9162

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

bufferflies
Copy link
Contributor

What problem does this PR solve?

Issue Number: Close #9161

What is changed and how does it work?

Check List

Tests

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

Code changes

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

Release note

None.

Signed-off-by: 童剑 <[email protected]>
Copy link
Contributor

ti-chi-bot bot commented Mar 21, 2025

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

@ti-chi-bot ti-chi-bot 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. dco-signoff: yes Indicates the PR's author has signed the dco. labels Mar 21, 2025
Copy link
Contributor

ti-chi-bot bot commented Mar 21, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from bufferflies, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 21, 2025
Signed-off-by: 童剑 <[email protected]>
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 21, 2025
Signed-off-by: 童剑 <[email protected]>
@bufferflies bufferflies marked this pull request as ready for review April 1, 2025 13:51
@ti-chi-bot 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 Apr 1, 2025
@bufferflies bufferflies force-pushed the scheduler/jobs branch 6 times, most recently from c4da339 to 74c5942 Compare April 2, 2025 02:47
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

Attention: Patch coverage is 60.60606% with 65 lines in your changes missing coverage. Please review.

Project coverage is 75.70%. Comparing base (7b5d47d) to head (5e3c20f).

❌ Your patch check has failed because the patch coverage (60.60%) is below the target coverage (74.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9162      +/-   ##
==========================================
- Coverage   75.76%   75.70%   -0.06%     
==========================================
  Files         476      476              
  Lines       73149    73285     +136     
==========================================
+ Hits        55421    55481      +60     
- Misses      14269    14323      +54     
- Partials     3459     3481      +22     
Flag Coverage Δ
unittests 75.70% <60.60%> (-0.06%) ⬇️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bufferflies bufferflies requested review from Copilot and lhy1024 April 2, 2025 07:14
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new API for the special scheduler that allows adding and managing jobs by renaming the configuration field from “role” to “rule” and extending job management endpoints while updating tests accordingly.

  • Renames the scheduler configuration field from role to rule.
  • Introduces new HTTP endpoints to add and delete scheduler jobs.
  • Updates tests and core logic to align with the new naming and job state changes.

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tools/pd-ctl/tests/scheduler/scheduler_test.go Test assertions updated to check for "rule" and new job status value.
tools/pd-ctl/pdctl/command/scheduler.go Updated input mapping from "role" to "rule".
server/api/scheduler.go Modified scheduler creation to delegate to job update if scheduler exists.
pkg/schedule/schedulers/balance_range.go Added job endpoints; updated job ID assignment and JSON marshalling.
pkg/schedule/schedulers/init.go Changed parameter handling from role to rule.
pkg/core/region.go Renamed Role to Rule with updated JSON encoder/decoder methods.
Other test and operator files Adjusted to reflect renaming and new job status handling.
Files not reviewed (1)
  • tools/pd-ut/testdata/group.3.etcdkey: Language not supported
Comments suppressed due to low confidence (1)

pkg/core/region.go:378

  • Using a pointer receiver for the String() method may lead to a nil pointer dereference; using a value receiver is safer if no state modification is required.
func (r *Rule) String() string {

@bufferflies bufferflies requested review from nolouch and Copilot April 2, 2025 07:15
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new API for scheduling jobs using a specialized scheduler by replacing occurrences of "role" with "rule" and adjusting behavior accordingly. It also introduces new endpoints for job addition and removal, and updates tests and configuration accordingly.

  • Updated pd-ctl commands to use "rule" instead of "role".
  • Modified API handlers to support job addition/expiration and updated configuration management.
  • Adjusted scheduler test cases and core types to align with the new naming.

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tools/pd-ctl/tests/scheduler/scheduler_test.go Test expectations updated to check for new "rule" and status "running".
tools/pd-ctl/pdctl/command/scheduler.go Changed input parameter from "role" to "rule".
server/api/scheduler.go Updated scheduler creation logic to forward to job update when scheduler exists.
pkg/schedule/schedulers/init.go Updated unescaping and conversion from "role" to "rule" but error messages still refer to "role".
pkg/schedule/schedulers/balance_range.go Added job addition and deletion API endpoints; new job ID calculation in addJob may be unsafe.
pkg/core/region.go Modified String and JSON functions for Rule from value receiver to pointer receiver.
Others Numerous changes across tests and operator influence to adopt the "rule" nomenclature.
Files not reviewed (1)
  • tools/pd-ut/testdata/group.3.etcdkey: Language not supported
Comments suppressed due to low confidence (3)

server/api/scheduler.go:123

  • When the scheduler exists and GetSchedulerConfigHandler returns nil (or err is nil), calling err.Error() could lead to a nil pointer dereference. Consider checking if err is nil before calling err.Error() or providing a default error message.
h.r.JSON(w, http.StatusNotAcceptable, err.Error())

pkg/schedule/schedulers/init.go:567

  • The error message still refers to "role" even though the new naming convention is "rule". Update the error message for consistency.
return errs.ErrQueryUnescape.FastGenByArgs("role")

pkg/core/region.go:378

  • Changing the String method to use a pointer receiver may lead to nil pointer dereferences if called on a non-pointer value. Consider using a value receiver or ensuring callers always use a pointer.
func (r *Rule) String() string {

@bufferflies
Copy link
Contributor Author

/test

Copy link
Contributor

ti-chi-bot bot commented Apr 9, 2025

@bufferflies: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test build
/test pull-integration-realcluster-test

The following commands are available to trigger optional jobs:

/debug pull-unit-test
/test pull-integration-copr-test

Use /test all to run the following jobs that were automatically triggered:

tikv/pd/ghpr_build
tikv/pd/pull_integration_realcluster_test

In response to this:

/test

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-sigs/prow repository.

@@ -360,12 +360,12 @@ func (r *RegionInfo) GetPeer(peerID uint64) *metapb.Peer {
return nil
}

// Role is the role of the region.
type Role int
// Rule is the rule for balance range scheduler
Copy link
Contributor

Choose a reason for hiding this comment

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

why change name by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the design used the rule not role. see pingcap/tidb#59681

Copy link
Contributor

@lhy1024 lhy1024 left a comment

Choose a reason for hiding this comment

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

If we replace role with rule, we also need to edit others

@@ -375,8 +375,8 @@ const (
)

// String returns the string value of the role.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some comments also need to be updated

Comment on lines +391 to +392
// NewRule creates a new role.
func NewRule(role string) Rule {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Comment on lines 426 to +427
// GetPeersByRole returns the peers with specified role.
func (r *RegionInfo) GetPeersByRole(role Role) []*metapb.Peer {
switch role {
func (r *RegionInfo) GetPeersByRole(rule Rule) []*metapb.Peer {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has signed the dco. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scheduler: add new api to add a new job for the special scheduler
2 participants