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

xds/internal/xdsclient/xdsresource: Preallocate VirtualHost slice correctly #7157

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

alingse
Copy link
Contributor

@alingse alingse commented Apr 21, 2024

when I want to add feature for linter makezero in PR ashanbrown/makezero#15 , I run a github action for real world repos, and the action result report this bug.

RELEASE NOTES: N/A

@aranjans
Copy link
Contributor

@alingse Thanks for writing in. While this change is functional, it's generally considered more efficient in Go to set an initial length of 0 and a capacity when using make with slices that will be appended to. This change looks good to me but we need to make sure there are no other place we are using the same as above.
@arvindbr8 Pitching you in to have a review from your side as well. This LGTM!

@alingse
Copy link
Contributor Author

alingse commented Apr 22, 2024

@aranjans I have run makezero linter for this project, and found only this bug. you can run it locally.

@arvindbr8
Copy link
Member

makezero is not a stylecheck we run on CI in this repo. We could consider this change if we decide to add this as part of our other stylechecks

@arvindbr8 arvindbr8 closed this Apr 22, 2024
@alingse
Copy link
Contributor Author

alingse commented Apr 22, 2024

It's not a style ok? It's a stupid bug @arvindbr8 have you ever really check this? @aranjans

截屏2024-04-23 00 29 00

the vhs have already pre-alloc with len(config.VirtualHosts) size, and then append it again.

if the VirtualHosts has three items, finally the vhs will be []VirtualHostWithInterceptors{ zeroValue, zeroValue,zeroValue , v1, v2, v3}

@arvindbr8 arvindbr8 reopened this Apr 23, 2024
@dfawley dfawley assigned zasweq and unassigned arvindbr8 Apr 23, 2024
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

LGTM

@zasweq zasweq requested a review from arvindbr8 April 23, 2024 18:33
@zasweq zasweq assigned arvindbr8 and unassigned zasweq Apr 23, 2024
@zasweq zasweq changed the title Fix miss makezero bug xds/internal/xdsclient/xdsresource: Preallocate VirtualHost slice correctly Apr 23, 2024
@zasweq zasweq added this to the 1.64 Release milestone Apr 23, 2024
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

In the first pass, I didnt realize this was a bug fix from the PR description. This looks good to me. Thanks for catching this.

@arvindbr8 arvindbr8 merged commit 5a24fc1 into grpc:master Apr 23, 2024
22 checks passed
@alingse
Copy link
Contributor Author

alingse commented Apr 24, 2024

In the first pass, I didnt realize this was a bug fix from the PR description. This looks good to me. Thanks for catching this.

sorry for the lack of detailed description in the PR. I took the lazy way out and directly copied the description from another PR. 😿

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants