Skip to content
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

bugfix: optimize routes status unnecessary deepcopy. #4527

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

qicz
Copy link
Member

@qicz qicz commented Oct 25, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Part Fixes #4516

Release Notes: Yes/No

@qicz qicz requested a review from a team as a code owner October 25, 2024 06:52
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 18.00000% with 82 lines in your changes missing coverage. Please review.

Project coverage is 65.75%. Comparing base (6f5ae8e) to head (5b95532).
Report is 118 commits behind head on main.

Files with missing lines Patch % Lines
internal/provider/kubernetes/status.go 14.73% 80 Missing and 1 partial ⚠️
internal/provider/kubernetes/status_updater.go 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4527      +/-   ##
==========================================
+ Coverage   65.72%   65.75%   +0.03%     
==========================================
  Files         211      211              
  Lines       31669    31623      -46     
==========================================
- Hits        20813    20795      -18     
+ Misses       9656     9627      -29     
- Partials     1200     1201       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@qicz
Copy link
Member Author

qicz commented Oct 25, 2024

/retest

@@ -66,16 +66,20 @@ func (r *gatewayAPIReconciler) subscribeAndUpdateStatus(ctx context.Context, ext
r.statusUpdater.Send(Update{
NamespacedName: key,
Resource: new(gwapiv1.HTTPRoute),
Mutator: MutatorFunc(func(obj client.Object) client.Object {
Mutator: MutatorFunc(func(obj client.Object) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the API is for mutating obects but is now returning a bool ? this doesnt seem right

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed this api to boolean

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the new definition for the mutator function

@zirain
Copy link
Contributor

zirain commented Oct 26, 2024

can you the memory diff before-after?

@qicz
Copy link
Member Author

qicz commented Oct 26, 2024

can you the memory diff before-after?

I think we should deal this after #4498 (comment)

That's comparable with the same way

}

// MutatorFunc is a function adaptor for Mutators.
type MutatorFunc func(client.Object) client.Object
type MutatorFunc func(client.Object) bool
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arkodg here

@qicz qicz requested a review from arkodg October 26, 2024 02:40
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

huge memory usage
3 participants