-
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
apf: call metrics.AddReject for decisionCancel #105739
Conversation
/assign @MikeSpreitzer @wojtek-t |
@@ -383,6 +383,7 @@ func (req *request) wait() (bool, bool) { | |||
req.removeFromQueueFn() | |||
qs.totRequestsWaiting-- | |||
metrics.AddRequestsInQueues(req.ctx, qs.qCfg.Name, req.fsName, -1) | |||
metrics.AddReject(req.ctx, qs.qCfg.Name, req.fsName, "time-out") |
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.
maybe we should use a new label "cancelled" to uniquely identify this scenario?
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.
Yes, I think that would be best.
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'm against adding a new label here. The "reason" label we currently have seems like a perfect fit for it.
It seems we're currently setting:
"concurrency-limit"
"queue-full"
Adding "timeout" seems like a good option here.
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 am fine with time-out
, we can make it unique in the future if there is a need.
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.
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.
Ohh - so basically in this line switching "time-out" to "cancelled", right?
Yeah - that sounds good to me.
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.
yes, I meant adding a new value cancelled
for the existing label reason
. I made the change.
/triage accepted |
d7b4798
to
f6dcf17
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tkashem, wojtek-t 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 bug
What this PR does / why we need it:
Which issue(s) this PR 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.: