-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Performance: remove legacy global edsClusters #21917
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
Conversation
@howardjohn @ramaraochavali It is ready now |
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.
(I reviewed istioctl changes only)
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.
LGTM, one minor question. But definitely would like more eyes.
con.mu.Unlock() | ||
} | ||
continue | ||
} | ||
|
||
previous := sets.NewSet(con.Clusters...) |
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.
I think sets was added just for this purpose - Do you want to retain them even if they are not used? I am OK to leave them like that as they could be used in else where in future.
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.
nice!
@howardjohn Can we see the enhancement somewhere? |
Please provide a description for what this PR is for.
remove legacy
edsClusters
to save cpu and memory.fix: #21913
And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.
[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[x] Networking
[x] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure