-
Notifications
You must be signed in to change notification settings - Fork 753
affinity: add base models and manager skeleton #9997
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
Signed-off-by: lhy1024 <admin@liudos.us>
4b84a5a to
9ba9b3a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9997 +/- ##
==========================================
- Coverage 78.53% 78.07% -0.47%
==========================================
Files 503 510 +7
Lines 67322 67900 +578
==========================================
+ Hits 52874 53013 +139
- Misses 10615 11045 +430
- Partials 3833 3842 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@bufferflies @okJiang PTAL |
| } | ||
|
|
||
| //nolint:revive | ||
| func (m *Manager) UpdateAffinityGroupPeers(string, uint64, []uint64) (*GroupState, error) { |
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.
| func (m *Manager) UpdateAffinityGroupPeers(string, uint64, []uint64) (*GroupState, error) { | |
| func (*Manager) UpdateAffinityGroupPeers(string, uint64, []uint64) (*GroupState, error) { |
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.
ok, but it will be used really in another pr later.
| if slice.HasDupInSorted(voterStoreIDs) { | ||
| return errs.ErrAffinityGroupContent.FastGenByArgs("duplicate voter store ID") | ||
| } | ||
| if !slices.Contains(voterStoreIDs, g.LeaderStoreID) { |
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 generative speaking, the leader should belong to the voters. How about renaming voterStoreIDs as followerStoreIDs if you don't allow the leader to exist in this list.
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.
Yes, we now only return error when LeaderStoreID is not in voterStoreIDs
pkg/schedule/affinity/group.go
Outdated
| } | ||
| for _, storeID := range voterStoreIDs { | ||
| if m.storeSetInformer.GetStore(storeID) == nil { | ||
| return errs.ErrAffinityGroupContent.FastGenByArgs("store does not exist") |
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.
How about informing the user that the store was not found?
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.
ok
| // GroupKeyRanges represents key ranges with group id. | ||
| type GroupKeyRanges struct { | ||
| KeyRanges []keyutil.KeyRange | ||
| GroupID string |
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.
How about adding more to the format of this groupID?
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.
Or we add it in pd-ctl about group format, I have added it in pd-ctl. We don't limit the format in server, it should be a convention for pd-ctl and SQL.
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 region label also does not conform to the server's specified format.
| re.NoError(err) | ||
|
|
||
| group := &Group{ | ||
| ID: "TEST-group_1", |
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.
affinity_group_id = tidb_p_t
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 only for test
Signed-off-by: lhy1024 <admin@liudos.us>
|
/cc @okJiang |
pkg/schedule/affinity/group.go
Outdated
| ) | ||
|
|
||
| // Phase is a status intended for API display | ||
| type Phase string |
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.
Moving Phase and condition to a new file is better for me.
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 it is not necessary for now. It will cause problems for subsequent code merges. Once all code has been merged, we can consider making further adjustments if necessary.
| // AffinityVer initializes at 1 and increments by 1 each time the Group changes. | ||
| AffinityVer uint64 | ||
| // AffinityRegionCount indicates how many Regions have all Voter and Leader peers in the correct stores. (AffinityVer equals). | ||
| AffinityRegionCount int |
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.
This is better.
| AffinityRegionCount int | |
| AffinitizedRegionCount int |
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.
affinity is also adj.
| // AffinityVer initializes at 1 and increments by 1 each time the Group changes. | ||
| AffinityVer uint64 | ||
| // AffinityRegionCount indicates how many Regions have all Voter and Leader peers in the correct stores. (AffinityVer equals). | ||
| AffinityRegionCount int |
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.
Why exist two same fields? L171-L173
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.
runtimeGroupInfo is struct for memory, GroupState is struct for api handler
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.
Why was it designed this way?
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.
How about renaming to test_utils.go
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.
Files ending with _test will only be compiled during test.
Signed-off-by: lhy1024 <admin@liudos.us>
[LGTM Timeline notifier]Timeline:
|
|
@niubell PTAL |
niubell
left a comment
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 first PR to add the basic structures, will add the corresponding tests in next PRs.
yes, they will be added in 9998 and 9999 |
|
@niubell: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bufferflies, niubell, okJiang 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 |
|
/test pull-error-log-review |
|
In response to a cherrypick label: new pull request created to branch |
| case storeDown: | ||
| return "down" | ||
| case storeRemovingOrRemoved: | ||
| return "removing-or-removed" |
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.
prefer to split them
| } | ||
|
|
||
| // IsAffinitySchedulingEnabled indicates whether affinity scheduling is allowed. | ||
| func (g *runtimeGroupInfo) IsAffinitySchedulingEnabled() bool { |
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 name is the same as IsAffinitySchedulingEnabled in the config, but the meaning is different.
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