-
Notifications
You must be signed in to change notification settings - Fork 1k
Update ReplicaEstimator interface signature, remove unncessary pointer type #6859
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
Update ReplicaEstimator interface signature, remove unncessary pointer type #6859
Conversation
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.
Pull Request Overview
Updates the ReplicaEstimator interface to change parameter types from pointer to value types, removing unnecessary pointer indirection for ComponentSetEstimationRequest and Component slices.
- Changed ComponentSetEstimationRequest parameter from pointer to value type in interface method
- Changed Components field from slice of pointers to slice of values
- Updated all implementing methods to match the new interface signature
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/estimator/client/interface.go | Updates interface signature and struct field to remove pointer types |
| pkg/estimator/client/general.go | Updates GeneralEstimator implementation to match new interface signature |
| pkg/estimator/client/accurate.go | Updates SchedulerEstimator implementation to match new interface signature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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 refines the 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the ReplicaEstimator interface signature by removing unnecessary pointer types for the ComponentSetEstimationRequest parameter in the MaxAvailableComponentSets method. This change affects the accurate.go, general.go, and interface.go files, ensuring that the estimator implementations use the value of the request object directly, rather than a pointer to it. This simplifies the code and reduces the risk of nil pointer dereferences.
d958a57 to
2743369
Compare
2743369 to
ed58e7b
Compare
|
cc @mszacillo PS: Please ignore the failing tests as the GitHub Action seems not stable today. I will handle it later. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6859 +/- ##
=======================================
Coverage 45.64% 45.64%
=======================================
Files 692 692
Lines 57703 57703
=======================================
+ Hits 26337 26339 +2
+ Misses 29720 29718 -2
Partials 1646 1646
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:
|
/retest |
Signed-off-by: RainbowMango <[email protected]>
ed58e7b to
a61df36
Compare
XiShanYongYe-Chang
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.
Thanks~
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: XiShanYongYe-Chang 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This pull request refines the ReplicaEstimator interface and its associated implementations by removing unnecessary pointer types for the
ComponentSetEstimationRequestand itsComponentsfield. This change aims to simplify the API and internal logic, making the code cleaner and potentially more efficient by avoiding pointer indirection where not strictly required for value types.Which issue(s) this PR fixes:
Part of #6734
Special notes for your reviewer:
My bad for suggesting using the pointer type at #6765 (comment).
I don't see any benefit from using a pointer type. :) See the inconvenience in #6857.
Does this PR introduce a user-facing change?: