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.

Sign up for GitHub

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

Jump to bottom

Refine profiling #874

Closed
wants to merge 11 commits into from
Closed

Refine profiling #874

wants to merge 11 commits into from

Conversation

shhdgit
Copy link
Member

@shhdgit shhdgit commented Mar 11, 2021

Some code refine about #859 (comment)

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@shhdgit
Copy link
Member Author

shhdgit commented Mar 11, 2021

/cc @baurine @breeswish

breezewish
breezewish reviewed Mar 18, 2021
View reviewed changes
Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

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

I'm also curious about how #717 can be implemented :) In #717, people want raw perf data, and different perf types..

pkg/apiserver/profiling/fetcher/client.go Outdated Show resolved Hide resolved
pkg/apiserver/profiling/module.go Outdated Show resolved Hide resolved
pkg/apiserver/profiling/profile.go Show resolved Hide resolved
pkg/apiserver/profiling/writer.go Show resolved Hide resolved
@breezewish
Copy link
Member

breezewish commented Mar 18, 2021 •
edited
Loading

Would you like a complete refactoring for this part, taking #717 into consideration, instead of improving the current code base? I guess it would be better. Remember to write a design doc first if you want to do this (i.e. refactoring / rewriting).

@shhdgit
Copy link
Member Author

shhdgit commented Mar 18, 2021

Thanks for such a helpful review!
Yes, I want to do this refactoring. I learned a lot from these missions.

@breezewish
Copy link
Member

Thanks for such a helpful review!
Yes, I want to do this refactoring. I learned a lot from these missions.

:) I usually find myself knowledge++ when throwing away code instead of producing code.

@shhdgit shhdgit force-pushed the refactor/profiling branch from 5b6cd66 to d78c663 Compare March 23, 2021 18:40
@shhdgit shhdgit marked this pull request as draft April 20, 2021 01:03
@breezewish breezewish force-pushed the master branch 2 times, most recently from b4a5d28 to ab56290 Compare July 9, 2021 10:30
@ti-chi-bot
Copy link
Member

@shhdgit: PR needs rebase.

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.

@breezewish
Copy link
Member

Profiling is now implementing by @YiniXu9506 . I think we can re-open this PR after that is finished.

@breezewish breezewish closed this Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breezewish breezewish breezewish left review comments

@baurine baurine Awaiting requested review from baurine

Assignees
No one assigned
Labels
needs-rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shhdgit @ti-chi-bot @breezewish