-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
remove unregistered event code #119539
remove unregistered event code #119539
Conversation
/ok-to-test |
/retest |
/retest |
1 similar comment
/retest |
/triage accepted |
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.
/lgtm
LGTM label has been added. Git tree hash: 9a5849d02e0be8403d2a6597a49bce598d630eb4
|
if newObj == nil { | ||
// Deletes don't make a pod schedulable. | ||
return framework.QueueSkip | ||
} |
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.
I agree that this is probably never reached, but I am on the edge whether the check should be removed outright.
If we never expect this to happen, should an error be logged when it occurs? Should the code panic?
If the check gets removed for performance reasons, then at least add a comment to the function that explains that newObj
will never be nil and why.
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.
Given that this will never happen, if an error occurs because of the removal of this code, then there is a problem with the code elsewhere and the problem should be exposed, not skip here, what do you think.
And i agree with adding a comment explains that newObj
will never be nil and why
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.
Okay, let's replace with a comment.
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.
I have updated the comment, please review
4bdfb02
to
986d205
Compare
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.
/lgtm
LGTM label has been added. Git tree hash: dac0e68a4ab2bd9aad001aa94a42046ad347b202
|
986d205
to
b20bc79
Compare
@@ -302,16 +302,11 @@ func (pl *dynamicResources) PreEnqueue(ctx context.Context, pod *v1.Pod) (status | |||
return nil | |||
} | |||
|
|||
// isSchedulableAfterClaimChange is invoked for all claim events reported by | |||
// isSchedulableAfterClaimChange is invoked for add and update claim events reported by |
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.
nit: to add
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.
Here it should be 'for', we call isSchedulableAfterClaimChange when we receive the add and update event.
/remove-sig node |
/assign @sanposhiho |
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.
/lgtm
/approve
LGTM label has been added. Git tree hash: 680362cc2e8d54b7eadc50214222ef06b3d72437
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HirazawaUi, pohly, sanposhiho 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:
We didn't register the delete event, this code will not work
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.: