Update grpc interface of multi-components set estimation #6939
Conversation
karmada-bot
added
the
kind/feature
karmada-bot
added
the
size/XL
Summary of ChangesHello @RainbowMango, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the gRPC interface for multi-component set estimation by introducing a more granular Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the multi-component set estimation interface by introducing a dedicated ComponentReplicaRequirements type and moving the namespace parameter from component-level to request-level. This change simplifies the API design by removing namespace from individual component requirements and instead passing it once at the request level.
Key changes:
- Introduced new
ComponentReplicaRequirementstype to replaceReplicaRequirementsin component definitions - Added
Namespacefield toMaxAvailableComponentSetsRequest - Updated all plugin interfaces and implementations to accept namespace as a parameter
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/estimator/pb/types.go | Introduced ComponentReplicaRequirements type; added namespace field to request |
| pkg/estimator/pb/generated.proto | Updated proto definitions for new types |
| pkg/estimator/pb/generated.pb.go | Auto-generated code for new proto definitions |
| pkg/estimator/server/framework/interface.go | Added namespace parameter to plugin interface methods |
| pkg/estimator/server/framework/runtime/framework.go | Threaded namespace parameter through plugin execution |
| pkg/estimator/server/framework/runtime/framework_test.go | Updated test to pass namespace parameter |
| pkg/estimator/server/framework/plugins/noderesource/noderesource.go | Added namespace parameter (unused) |
| pkg/estimator/server/framework/plugins/noderesource/noderesource_test.go | Updated tests with new type and namespace parameter |
| pkg/estimator/server/framework/plugins/resourcequota/resourcequota.go | Uses namespace from parameter instead of component field |
| pkg/estimator/server/framework/plugins/resourcequota/resourcequota_test.go | Updated tests with new type and namespace field |
| pkg/estimator/server/estimate.go | Passes namespace from request to estimation method |
| pkg/estimator/client/accurate.go | Builds request with namespace at request level |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/estimator/pb/types.go
Outdated
| Replicas int32 `json:"replicas" protobuf:"varint,3,opt,name=replicas"` | ||
| } | ||
|
|
||
| // ComponentReplicaRequirements represents the requirements required by each replica. |
There was a problem hiding this comment.
The documentation comment is redundant ('requirements required'). Consider rephrasing to 'ComponentReplicaRequirements represents the resource and scheduling requirements for each replica.'
| // ComponentReplicaRequirements represents the requirements required by each replica. | |
| // ComponentReplicaRequirements represents the resource and scheduling requirements for each replica. |
There was a problem hiding this comment.
Updated as suggested.
But hope to start another PR to fix the other places.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the gRPC interface for multi-component set estimation by introducing a dedicated ComponentReplicaRequirements message and moving the namespace field to the top-level request. The changes are consistently applied across the client, server, framework, and plugins, improving the API design. I've identified a potential issue with handling an empty namespace in the resource quota estimator and suggested improvements for comment clarity in the protobuf definitions.
RainbowMango
force-pushed
the
pr_optimize_estimator_grpc
branch
from
58f128a to
52aaeba
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6939 +/- ##
==========================================
- Coverage 46.45% 46.44% -0.01%
==========================================
Files 698 698
Lines 47809 47811 +2
==========================================
Hits 22208 22208
- Misses 23930 23932 +2
Partials 1671 1671
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| namespace := components[0].ReplicaRequirements.Namespace | ||
| if namespace == "" { | ||
| klog.V(5).Infof("%s: namespace is empty, skipping resource quota check", pl.Name()) | ||
| return noQuotaConstraint, framework.NewResult(framework.Success) |
There was a problem hiding this comment.
Thanks! LGTM overall.
Only one thing from me, after checking the nooperation definition: Nooperation is used when a plugin is disabled or the plugin list is empty, I think we might need to change the status code on line 145 to Success.
Could you help change it in this PR? Thanks!
There was a problem hiding this comment.
Sorry, forgot to mention that we also need to change the test here.
Could you help fix it? Thanks!
There was a problem hiding this comment.
Oh, my bad.
Updated. It should be fine now.
# go test ./pkg/estimator/server/framework/plugins/resourcequota/...
ok github.com/karmada-io/karmada/pkg/estimator/server/framework/plugins/resourcequota
|
/assign |
RainbowMango
force-pushed
the
pr_optimize_estimator_grpc
branch
from
52aaeba to
a975f2e
Compare
Signed-off-by: RainbowMango <[email protected]>
RainbowMango
force-pushed
the
pr_optimize_estimator_grpc
branch
from
a975f2e to
71d335a
Compare
|
/lgtm |
RainbowMango
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
This pull request refactors the gRPC interface for multi-component set estimation by introducing a dedicated
ComponentReplicaRequirementsmessage and moving thenamespacefield to the top-level request. The changes are consistently applied across the client, server, framework, and plugins, improving the API design.Which issue(s) this PR fixes:
Fixes #
Part of #6734
Special notes for your reviewer:
Does this PR introduce a user-facing change?: