Skip to content

Conversation

hzxuzhonghu
Copy link
Member

@hzxuzhonghu hzxuzhonghu commented Mar 6, 2020

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

@hzxuzhonghu hzxuzhonghu requested review from a team as code owners March 6, 2020 06:35
@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 6, 2020
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Mar 6, 2020
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 6, 2020
@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 9, 2020
@hzxuzhonghu hzxuzhonghu changed the title [WIP] Performance: remove legacy global edsClusters Performance: remove legacy global edsClusters Mar 9, 2020
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 9, 2020
@hzxuzhonghu
Copy link
Member Author

@howardjohn @ramaraochavali It is ready now

Copy link
Contributor

@esnible esnible left a 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)

Copy link
Contributor

@ramaraochavali ramaraochavali left a 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...)
Copy link
Contributor

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.

Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

nice!

@istio-testing istio-testing merged commit 19ec254 into istio:master Mar 10, 2020
@hzxuzhonghu hzxuzhonghu deleted the rm-edsCluster branch March 11, 2020 00:26
@hzxuzhonghu
Copy link
Member Author

@howardjohn Can we see the enhancement somewhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider cleanup global cache edsClusters
6 participants