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
use klog.InfoS instead of klog.V(0).InfoS #111708
Conversation
4378b93
to
ae8e86b
Compare
/sig instrumentation |
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.
/test pull-kubernetes-e2e-kind
@yangjunmyfm192085 always remember to address the "why" part because that gives a reviewer, often someone who is busy and may lack the necessary background information, an incentive to look at the PR. In this case it is "... because they are semantically equivalent and V(0) add some (small) overhead". I prefer to not merge this until we have a way to enforce that this code pattern doesn't get added back in some future PR (kubernetes-sigs/logtools#2). |
/triage accepted |
ae8e86b
to
2db4dea
Compare
Hi, @pohly, Thanks for the reminder, I added the reason for the modification above. |
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.
/retest
If we do it in that order, we may have to do the same fixup multiple times because new code adds the pattern back. Let's avoid that. |
/hold |
I opened a pr kubernetes-sigs/logtools#4 to add checking for logs |
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.
Hi, @pohly , Could we consider merge this pr now?
Of course, I will continue to analyze other errors in logcheck.
Yes, let's merge this now. Releasing a new logcheck is taking a bit longer, so enabling the check will have to wait. /hold cancel |
/assign @smarterclayton For approval. |
/cc @liggitt |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, yangjunmyfm192085 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
use klog.InfoS instead of klog.V(0).InfoS
As mentioned in the issue kubernetes-sigs/logtools#2, Calling V with zero as parameter just causes additional overhead, so we need to avoid this scenario
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/cc @pohly