Conversation
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
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 @ystaticy. 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. |
ti-chi-bot
bot
added
the
size/L
📝 WalkthroughWalkthroughAdds RPC error classification and a local degraded-token-bucket fallback path used when AcquireTokenBuckets RPCs fail; integrates degraded response generation into the token-bucket request flow and adds tests for classification and fallback behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceGroupsController as Controller
participant TokenBucketService as RPC_Server
participant LocalFallback as Fallback_Generator
Client->>Controller: Request token buckets
Controller->>RPC_Server: AcquireTokenBuckets RPC
alt RPC success
RPC_Server-->>Controller: TokenBucketResponses
Controller-->>Client: Return responses
else RPC transport error (Unavailable/DeadlineExceeded/...)
RPC_Server--x Controller: RPC error
Controller->>Fallback_Generator: buildDegradedTokenBucketResponses(requests, degradedSettings)
Fallback_Generator-->>Controller: Degraded TokenBucketResponses
Controller-->>Client: Return degraded responses (logs info, records latency)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
client/resource_group/controller/global_controller_test.go (1)
539-540: Replace fixed sleeps with eventual assertions to reduce flakiness.Line 539 and Line 591 use hard sleeps for async state propagation; this can be unstable in slower CI environments.
Example refactor pattern
- time.Sleep(500 * time.Millisecond) + require.Eventually(t, func() bool { + reqCtx, reqCancel := context.WithTimeout(ctx, 200*time.Millisecond) + defer reqCancel() + requestInfo := NewTestRequestInfo(false, 0, 1, AccessUnknown) + _, _, _, _, err := controller.OnRequestWait(reqCtx, defaultResourceGroupName, requestInfo) + return err == nil + }, 3*time.Second, 50*time.Millisecond)- time.Sleep(500 * time.Millisecond) + require.Eventually(t, func() bool { + return gc.run.requestInProgress + }, 3*time.Second, 50*time.Millisecond)Also applies to: 591-592
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/resource_group/controller/global_controller_test.go` around lines 539 - 540, Replace the fixed time.Sleep(500 * time.Millisecond) (and the similar sleeps at lines ~591-592) with an eventual/polling assertion that repeatedly checks the async state change until a timeout; for example use wait.PollImmediate/require.Eventually (or your test util) to poll the relevant condition (the resource status update or controller side-effect the test expects) instead of sleeping, referencing the test in global_controller_test.go that currently uses time.Sleep so the loop queries the same function/field used in the later assertion and fails only after a bounded timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/resource_group/controller/global_controller_test.go`:
- Around line 539-540: Replace the fixed time.Sleep(500 * time.Millisecond) (and
the similar sleeps at lines ~591-592) with an eventual/polling assertion that
repeatedly checks the async state change until a timeout; for example use
wait.PollImmediate/require.Eventually (or your test util) to poll the relevant
condition (the resource status update or controller side-effect the test
expects) instead of sleeping, referencing the test in global_controller_test.go
that currently uses time.Sleep so the loop queries the same function/field used
in the later assertion and fails only after a bounded timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0388f83-0d4f-4b3a-851d-39cc48871be1
📒 Files selected for processing (2)
client/resource_group/controller/global_controller.goclient/resource_group/controller/global_controller_test.go
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Linking issues" section in the CONTRIBUTING.md. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/resource_group/controller/global_controller_test.go (1)
453-484: LGTM!Good test coverage for
buildDegradedTokenBucketResponses. The nil-settings case is tested, and the response shape is validated including settings propagation and TrickleTimeMs.💡 Optional: Additional nil edge cases
Consider adding tests for partial nil cases (nil
RUor nilRU.Settings) to fully exercise the guard at line 527 of the implementation:// Nil RU -> nil response re.Nil(buildDegradedTokenBucketResponses(requests, &rmpb.GroupRequestUnitSettings{RU: nil})) // Nil Settings -> nil response re.Nil(buildDegradedTokenBucketResponses(requests, &rmpb.GroupRequestUnitSettings{ RU: &rmpb.TokenBucket{Settings: nil}, }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/resource_group/controller/global_controller_test.go` around lines 453 - 484, Add two additional unit cases to TestBuildDegradedTokenBucketResponses to cover partial-nil inputs: call buildDegradedTokenBucketResponses with &rmpb.GroupRequestUnitSettings{RU: nil} and with &rmpb.GroupRequestUnitSettings{RU: &rmpb.TokenBucket{Settings: nil}} and assert the result is nil for each; these tests target the nil-guard logic inside buildDegradedTokenBucketResponses and ensure it properly handles missing RU or missing TokenBucket.Settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/resource_group/controller/global_controller_test.go`:
- Around line 453-484: Add two additional unit cases to
TestBuildDegradedTokenBucketResponses to cover partial-nil inputs: call
buildDegradedTokenBucketResponses with &rmpb.GroupRequestUnitSettings{RU: nil}
and with &rmpb.GroupRequestUnitSettings{RU: &rmpb.TokenBucket{Settings: nil}}
and assert the result is nil for each; these tests target the nil-guard logic
inside buildDegradedTokenBucketResponses and ensure it properly handles missing
RU or missing TokenBucket.Settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e22cdc0-a012-44e6-bec2-e63f11de97a9
📒 Files selected for processing (2)
client/resource_group/controller/global_controller.goclient/resource_group/controller/global_controller_test.go
1 participant
What problem does this PR solve?
Issue Number: Close #xxx
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests