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

Simplify checking in getMinTolerationTime #79443

Merged
merged 1 commit into from Aug 5, 2019

Conversation

tedyu
Copy link
Contributor

@tedyu tedyu commented Jun 26, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
In getMinTolerationTime (taint_manager.go),

			} else if tolerationSeconds < minTolerationTime || minTolerationTime == -1 {

when tolerationSeconds >= minTolerationTime, even if minTolerationTime has been set in prior iteration (!= -1), the second clause would still be evaluated.

After my PR, the wasteful check doesn't exist.

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 26, 2019
@tedyu
Copy link
Contributor Author

tedyu commented Jun 26, 2019

@mborsz
Can you take a look ?

@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 26, 2019
@draveness
Copy link
Contributor

/lgtm
/priority backlog

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 27, 2019
@tedyu
Copy link
Contributor Author

tedyu commented Jul 26, 2019

ping @bowei for review

@bowei bowei removed their assignment Jul 29, 2019
@bowei
Copy link
Member

bowei commented Jul 29, 2019

Can you couple this with a bugfix? This patch just fixes a single spelling mistake.

@tedyu
Copy link
Contributor Author

tedyu commented Jul 29, 2019

@bowei
Correcting typo alone can be found in git log.
e.g.
#80524

To my knowledge, the underlying code doesn't have bug.

@tedyu
Copy link
Contributor Author

tedyu commented Jul 30, 2019

@bowei

git log | grep typo | wc
    2194   12163   94222

The above is count of typo fixes

@tedyu
Copy link
Contributor Author

tedyu commented Jul 30, 2019

@bowei
In getMinTolerationTime (taint_manager.go),

			} else if tolerationSeconds < minTolerationTime || minTolerationTime == -1 {

I can push up one change which optimizes out the 'minTolerationTime == -1' check by changing initial value for minTolerationTime

Let me know what you think.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2019
@tedyu tedyu changed the title Correct typo for TaintManager Simplify checking in getMinTolerationTime Jul 30, 2019
@tedyu
Copy link
Contributor Author

tedyu commented Jul 30, 2019

/test pull-kubernetes-integration

@tedyu
Copy link
Contributor Author

tedyu commented Jul 30, 2019

/test pull-kubernetes-e2e-gce-100-performance

2 similar comments
@tedyu
Copy link
Contributor Author

tedyu commented Jul 30, 2019

/test pull-kubernetes-e2e-gce-100-performance

@tedyu
Copy link
Contributor Author

tedyu commented Jul 31, 2019

/test pull-kubernetes-e2e-gce-100-performance

@tedyu
Copy link
Contributor Author

tedyu commented Jul 31, 2019

@bowei
We should fix the original condition I listed above because when tolerationSeconds >= minTolerationTime, even if minTolerationTime has been set in prior iteration (!= -1), the second clause would still be evaluated.

After my PR, the wasteful check doesn't exist.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2019
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 31, 2019
@tedyu
Copy link
Contributor Author

tedyu commented Jul 31, 2019

/test pull-kubernetes-e2e-gce

@tedyu
Copy link
Contributor Author

tedyu commented Aug 2, 2019

@bowei
Can you take another look ?

@bowei
Copy link
Member

bowei commented Aug 5, 2019

add test cases in the unit tests for

  1. multiple toleration times
  2. -1 behavior

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 5, 2019
@tedyu
Copy link
Contributor Author

tedyu commented Aug 5, 2019

@bowei
Your comment has been addressed.

@bowei
Copy link
Member

bowei commented Aug 5, 2019

/lgtm
/approve
/remove-hold

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2019
@bowei
Copy link
Member

bowei commented Aug 5, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 5, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, tedyu

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2019
@k8s-ci-robot k8s-ci-robot merged commit 9e735d6 into kubernetes:master Aug 5, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. 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