-
Notifications
You must be signed in to change notification settings - Fork 753
api: add affinity groups api and client support #10041
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
Conversation
|
Skipping CI for Draft Pull Request. |
068e513 to
fd696e6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10041 +/- ##
==========================================
+ Coverage 78.40% 78.49% +0.09%
==========================================
Files 511 513 +2
Lines 68415 68893 +478
==========================================
+ Hits 53644 54081 +437
- Misses 10876 10899 +23
- Partials 3895 3913 +18
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
fd696e6 to
31db939
Compare
Signed-off-by: lhy1024 <admin@liudos.us>
31db939 to
137d358
Compare
|
/retest |
1 similar comment
|
/retest |
|
|
||
| // IsStable indicates that the Group has completed the required scheduling and is currently in a stable state. | ||
| func (s *AffinityGroupState) IsStable() bool { | ||
| return s.Phase == "stable" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The phase name is strange, but we can rename it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is public. Can we rename it in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I means "Can" we update it?
server/apiv2/handlers/affinity.go
Outdated
| router.PATCH("", BatchModifyAffinityGroups) | ||
| router.PUT("/:group_id", UpdateAffinityGroupPeers) | ||
| router.DELETE("/:group_id", DeleteAffinityGroup) | ||
| router.POST("/batch-delete", BatchDeleteAffinityGroups) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not meet the RESTful requirement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary reason is that we want to perform batch deletions within a single transaction, but the DELETE does not support carrying a body.
And I found that there is also "/config/rules/batch" and "/regions/accelerate-schedule/batch", so I chose a similar path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just find an example https://linter.aip.dev/235/http-method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion, use POST /?delete to batch delete https://docs.aws.amazon.com/AmazonS3/latest/userguide/delete-multiple-objects.html
Signed-off-by: lhy1024 <admin@liudos.us>
|
|
||
| // IsStable indicates that the Group has completed the required scheduling and is currently in a stable state. | ||
| func (s *AffinityGroupState) IsStable() bool { | ||
| return s.Phase == "stable" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I means "Can" we update it?
There was a problem hiding this 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 comprehensive API and client support for affinity groups, which allow managing key ranges with specific peer placement requirements for region scheduling. The feature enables users to group key ranges together and control their leader and voter store placement through a RESTful API.
Key Changes
- Adds complete REST API endpoints for affinity group management (create, get, update, delete, batch operations)
- Implements HTTP client methods for all affinity group operations
- Adds middleware to check if affinity scheduling is enabled before processing requests
- Includes comprehensive test coverage for API handlers and client integration
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/apiv2/router.go | Registers affinity group API handlers with the v2 router |
| server/apiv2/middlewares/affinity_enabled_checker.go | New middleware to block requests when affinity scheduling is disabled |
| server/apiv2/handlers/affinity.go | Complete handler implementation for all affinity group API endpoints including create, get, update, delete, and batch modify operations |
| client/http/types.go | Defines client-side types for affinity group operations and responses |
| client/http/interface.go | Adds affinity group methods to the HTTP client interface and implements all operations |
| client/http/api.go | Adds API endpoint path constants for affinity groups |
| pkg/utils/apiutil/apiutil.go | Adds PutJSON utility function for PUT requests |
| pkg/utils/testutil/api_check.go | Adds CheckPutJSON helper for testing PUT endpoints |
| tests/server/apiv2/handlers/affinity_test.go | Comprehensive test suite covering all affinity group operations, error cases, and edge cases |
| tests/integrations/client/http_client_test.go | Integration tests for the HTTP client affinity group methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
|
|
||
| // Convert internal manager output to API output. | ||
| resp := AffinityGroupsResponse{ | ||
| AffinityGroups: make(map[string]*affinity.GroupState, len(req.AffinityGroups)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question. Is it necessary to return complete Group information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, we will be able to include the correct StoreIDs in the return value of Create, which will help tidb update the cache faster.
Signed-off-by: lhy1024 <admin@liudos.us>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: okJiang, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ref tikv#9764 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
|
@lhy1024: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
ref tikv#9764 Signed-off-by: lhy1024 <admin@liudos.us>
ref tikv#9764 Signed-off-by: lhy1024 <admin@liudos.us>
ref tikv#9764 Signed-off-by: lhy1024 <admin@liudos.us>
ref tikv#9764 Signed-off-by: lhy1024 <admin@liudos.us>
ref tikv#9764 Signed-off-by: lhy1024 <admin@liudos.us>
ref tikv#9764 Signed-off-by: lhy1024 <admin@liudos.us>
ref tikv#9764 Signed-off-by: lhy1024 <admin@liudos.us>
What problem does this PR solve?
Issue Number: Ref #9764
What is changed and how does it work?
Check List
Tests
Release note