resource_group client: narrow degraded fallback conditions for GetResourceGroup#11009
Open
ystaticy wants to merge 4 commits into
Open
resource_group client: narrow degraded fallback conditions for GetResourceGroup#11009ystaticy wants to merge 4 commits into
ystaticy wants to merge 4 commits into
Conversation
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
dco-signoff: yes
Contributor
|
[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
the
size/XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change preserves underlying resource-group errors, maps gRPC cancellation and deadline statuses to context errors, and updates controller fallback decisions to classify errors before creating degraded resource groups. ChangesResource group error handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GlobalController
participant ResourceManagerClient
participant DegradedResourceGroup
Caller->>GlobalController: request resource-group controller
GlobalController->>ResourceManagerClient: GetResourceGroup(context)
ResourceManagerClient-->>GlobalController: wrapped error
GlobalController->>GlobalController: normalize and classify error
GlobalController->>DegradedResourceGroup: create degraded group when eligible
GlobalController-->>Caller: degraded controller or normalized error
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🤖 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 `@client/resource_group/controller/global_controller_test.go`:
- Around line 342-345: Update the newController helper to accept the subtest
testing.T and create a local require.New(t) within each invocation instead of
using the parent re. Update every t.Run call that invokes newController to pass
its subtest t, preserving the existing controller creation and assertion
behavior.
In `@client/resource_group/controller/global_controller.go`:
- Around line 579-580: Update the error handling around
normalizeGetResourceGroupError to check context.Canceled and
context.DeadlineExceeded on the original error before normalization. Then
classify the unwrapped gRPC status so codes.DeadlineExceeded reaches the
existing degraded-mode path, while preserving the current cancellation handling.
In `@client/resource_manager_client.go`:
- Around line 162-173: Update the error mapping in
client/resource_manager_client.go:162-173 around the resource-group client
method to convert codes.DeadlineExceeded to ctx.Err() only when the caller
context is done; otherwise preserve the original gRPC deadline error. Ensure
client/resource_group/controller/global_controller.go:579-588 continues routing
server-generated deadline errors through the degraded fallback, and add coverage
in client/resource_group/controller/global_controller_test.go:397-425 for a
server-returned codes.DeadlineExceeded case.
🪄 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
Run ID: 80d51619-73e1-4ed0-9026-61aacf4a2b6e
📒 Files selected for processing (5)
client/errs/errno.goclient/resource_group/controller/global_controller.goclient/resource_group/controller/global_controller_test.goclient/resource_manager_client.goclient/resource_manager_client_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11009 +/- ##
========================================
Coverage 79.20% 79.21%
========================================
Files 541 541
Lines 75450 76039 +589
========================================
+ Hits 59762 60236 +474
- Misses 11456 11549 +93
- Partials 4232 4254 +22
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ystaticy
changed the title
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Contributor
Author
|
/test pull-unit-test-next-gen-2 |
Contributor
Author
|
/retest-required |
1 similar comment
Contributor
Author
|
/retest-required |
Contributor
Author
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #11010
What is changed and how does it work?
Refine the degraded fallback behavior of
GetResourceGroupwhendegradedRUSettingsis configured.Currently, degraded fallback is too broad and may hide original errors from RM. We should only use degraded fallback for transient RM failures, while preserving original errors for non-retryable cases.
Expected behavior:
SwitchGrouptarget -> do not switchIt is also helpful to preserve the underlying error from
ErrClientGetResourceGroupso upper layers can correctly inspect errors such ascontext.Canceledandcontext.DeadlineExceeded.Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
CanceledandDeadlineExceedederrors reliably, including correct handling when the server returns gRPC deadline failures.Tests