scheduler, evict-slow-store allow draining multiple store from AZ#11047
scheduler, evict-slow-store allow draining multiple store from AZ#11047HaoW30 wants to merge 5 commits into
Conversation
Signed-off-by: artem_danilov <artem.danilov@airbnb.com>
Signed-off-by: artem_danilov <artem.danilov@airbnb.com>
Signed-off-by: artem_danilov <artem.danilov@airbnb.com>
ti-chi-bot
Bot
added
the
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
contribution
|
Hi @HaoW30. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
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. |
ti-chi-bot
Bot
added
the
size/XXL
📝 WalkthroughWalkthroughThe ChangesGrouped slow-store eviction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StoreHeartbeat
participant EvictSlowStoreScheduler
participant ClusterStore
StoreHeartbeat->>EvictSlowStoreScheduler: Update slow-store score
EvictSlowStoreScheduler->>ClusterStore: Inspect labels and healthy stores
ClusterStore-->>EvictSlowStoreScheduler: Return domain and topology state
EvictSlowStoreScheduler->>EvictSlowStoreScheduler: Select compatible eviction group
EvictSlowStoreScheduler->>ClusterStore: Persist evicted stores and drain leaders
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/schedule/schedulers/evict_slow_store.go`:
- Around line 573-576: Update cleanupEvictLeader’s error path after
clearEvictedAndPersist to include the returned err in the log entry, while
retaining the existing store-ids field and message context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7eeeffa8-25d0-4aff-8571-4cab918129b6
📒 Files selected for processing (3)
pkg/schedule/schedulers/evict_slow_store.gopkg/schedule/schedulers/evict_slow_store_test.goserver/cluster/cluster_test.go
| evictSlowStores, err := s.conf.clearEvictedAndPersist() | ||
| if err != nil { | ||
| log.Info("evict-slow-store-scheduler persist config failed", zap.Uint64("store-id", evictSlowStore)) | ||
| log.Info("evict-slow-store-scheduler persist config failed", zap.Uint64s("store-ids", evictSlowStores)) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log the persist error in cleanupEvictLeader. On failure clearEvictedAndPersist returns nil, err, so this log emits empty store-ids and, more importantly, drops the actual error — making a real persistence failure hard to diagnose.
🪵 Include the error
evictSlowStores, err := s.conf.clearEvictedAndPersist()
if err != nil {
- log.Info("evict-slow-store-scheduler persist config failed", zap.Uint64s("store-ids", evictSlowStores))
+ log.Info("evict-slow-store-scheduler persist config failed", zap.Uint64s("store-ids", evictSlowStores), zap.Error(err))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| evictSlowStores, err := s.conf.clearEvictedAndPersist() | |
| if err != nil { | |
| log.Info("evict-slow-store-scheduler persist config failed", zap.Uint64("store-id", evictSlowStore)) | |
| log.Info("evict-slow-store-scheduler persist config failed", zap.Uint64s("store-ids", evictSlowStores)) | |
| } | |
| evictSlowStores, err := s.conf.clearEvictedAndPersist() | |
| if err != nil { | |
| log.Info("evict-slow-store-scheduler persist config failed", zap.Uint64s("store-ids", evictSlowStores), zap.Error(err)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/schedule/schedulers/evict_slow_store.go` around lines 573 - 576, Update
cleanupEvictLeader’s error path after clearEvictedAndPersist to include the
returned err in the log entry, while retaining the existing store-ids field and
message context.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11047 +/- ##
==========================================
+ Coverage 79.16% 79.29% +0.12%
==========================================
Files 536 542 +6
Lines 73609 76271 +2662
==========================================
+ Hits 58273 60479 +2206
- Misses 11235 11526 +291
- Partials 4101 4266 +165
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Pls check the previous unresolved comments in the original PR, thx |
3 participants
What problem does this PR solve?
Issue Number: Close #10742
This is to address the comment for PR #10743 while the original author is away.
Please reference the original PR for previous discussions.
What is changed and how does it work?
Check List
Tests
Please see #10743
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
New Features
Bug Fixes