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

Skip some analyzers for in-cluster analysis to reduce allocations #41249

Closed
wants to merge 3 commits into from

Conversation

hanxiaop
Copy link
Member

@hanxiaop hanxiaop commented Oct 3, 2022

Please provide a description of this PR:
Related to #30200, will need a better way to handle all the analyzers properly.

@hanxiaop hanxiaop requested a review from a team as a code owner October 3, 2022 14:26
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 3, 2022
@istio-testing
Copy link
Collaborator

@hanxiaop: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
release-notes_istio fda85af link true /test release-notes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

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.

This isn't really fixing the issue, just working around... what this is saying is the architecture of analyze is broken so we are going to continually chip away at its functionality until it is far less useful.

How many analyzers are we going to remove? I see this one just removes 2, but if another report comes in will there be 3 removed? 10?

@hanxiaop
Copy link
Member Author

hanxiaop commented Oct 3, 2022

This isn't really fixing the issue, just working around... what this is saying is the architecture of analyze is broken so we are going to continually chip away at its functionality until it is far less useful.

How many analyzers are we going to remove? I see this one just removes 2, but if another report comes in will there be 3 removed? 10?

@howardjohn Yes looks like it's not a great idea in the long term. We should refactor the analyze to avoid of analyzing same configs all the time if the feature is enabled. I can look into it since there's no activity in the issue for a long time. BTW i think we can temporarily remove these two analyzers since they are not quite useful for the in-cluster analysis.

@hanxiaop hanxiaop closed this Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking area/user experience size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants