Updated error responses around rate limiting to be more descriptive by rraavi · Pull Request #4972 · apache/openwhisk

rraavi · 2020-09-09T15:15:09Z

Description

Addressing issue #4798. Minor update.

Related issue and scope

I opened an issue to propose and discuss this change (#????)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
Enhancement or new feature (adds new functionality).
Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

I signed an Apache CLA.
I reviewed the style guides and followed the recommendations (Travis CI will check :).
I added tests to cover my changes.
My changes require further changes to the documentation.
I updated the documentation where necessary.

rraavi · 2020-09-09T16:09:47Z

Waiting on a stable master, converting PR to draft till then :)

bdoyle0182 · 2020-09-09T18:50:29Z

common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala

  /** Standard message for too many activation requests within a rolling time window. */
  def tooManyRequests(count: Int, allowed: Int) =
-    s"Too many requests in the last minute (count: $count, allowed: $allowed)."
+    s"Too many requests in the last minute (currently running: $count, allowed per minute: $allowed)."


This shouldn't be currently running as it's just a per minute throttle limit, not concurrency.

Reverted in dc3ec0f

rraavi · 2020-09-10T20:33:14Z

@dgrove-oss @bdoyle0182 @mrutkows tagging folks for review!

Note: Travis timed out running ./tools/travis/checkAndUploadLogs.sh system command for System Tests job (I couldn't see a way to kick off a new CI build manually)

mrutkows · 2020-09-11T18:35:25Z

Is there a way to also show the discrete # (perhaps use the same internal config. value the controller uses) to be as descriptive as possible? That is fully address Dave's suggestion in the issue, i.e., "or simply show the total allowed over all the controllers (in this case, 5 controllers (5*240=1200)". I suspect operators would love to be reminded of the configured limits when a user presents them with this error...

bdoyle0182 · 2020-09-15T21:50:35Z

Is there a way to also show the discrete # (perhaps use the same internal config. value the controller uses) to be as descriptive as possible? That is fully address Dave's suggestion in the issue, i.e., "or simply show the total allowed over all the controllers (in this case, 5 controllers (5*240=1200)". I suspect operators would love to be reminded of the configured limits when a user presents them with this error...

I agree I think this is probably the biggest thing we want as to what's misleading. I'm not sure if the current clustering can actually support something like this though or not.

…scriptive

rraavi · 2020-09-26T00:43:28Z

Is there a way to also show the discrete # (perhaps use the same internal config. value the controller uses) to be as descriptive as possible? That is fully address Dave's suggestion in the issue, i.e., "or simply show the total allowed over all the controllers (in this case, 5 controllers (5*240=1200)". I suspect operators would love to be reminded of the configured limits when a user presents them with this error...

Team, unfortunately this is my first time working with Scala.. I tried to follow along the code but wasn't entirely sure. Are we referring to leverage clusterSize property of LoadBalancer to compute and show the discrete count? I noticed another property named totalActiveActivations.. is this something we can use?

dgrove-oss · 2020-09-29T16:11:48Z

common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala

  /** Standard message for too many concurrent activation requests within a time window. */
  def tooManyConcurrentRequests(count: Int, allowed: Int) =
-    s"Too many concurrent requests in flight (count: $count, allowed: $allowed)."
+    s"Too many concurrent requests in flight (currently running: $count, allowed per controller: $allowed)."


Suggested change

s"Too many concurrent requests in flight (currently running: $count, allowed per controller: $allowed)."

s"Too many concurrent requests (count in window: $count, allowed per controller: $allowed)."

Suggest a change to address @bdoyle0182 's comment

@dgrove-oss @bdoyle0182 @mrutkows suggested a change in a separate PR linked below to keep it clean. I'll just close the other PR if I'm on the wrong track ;)

@rabbah - do you have any thoughts? This is a part of the controller I don't know very well.

rabbah · 2020-10-07T14:26:42Z

As noted by @dgrove-oss and @bdoyle0182 each controller lacks total knowledge about the activation counts per namespace. This message will even vary per controller depending on which services the request. So the "currently running" count is for the specific controller that was assigned the activation.

mrutkows

LGTM... achieves immediacy of ack. multiple controllers.

rraavi · 2020-10-09T19:45:40Z

So, close the linked PR right?

style95 · 2020-11-14T12:27:24Z

I have little idea about the history of this PR.
Is this ready to merge?

rraavi marked this pull request as draft September 9, 2020 16:09

bdoyle0182 requested changes Sep 9, 2020

View reviewed changes

rraavi force-pushed the update-error-responses branch 3 times, most recently from 7e61b7c to c32ec0d Compare September 10, 2020 17:15

rraavi marked this pull request as ready for review September 10, 2020 18:14

rraavi marked this pull request as draft September 10, 2020 18:19

rraavi force-pushed the update-error-responses branch from c32ec0d to 4f08b4c Compare September 10, 2020 19:10

rraavi marked this pull request as ready for review September 10, 2020 20:28

rraavi force-pushed the update-error-responses branch from 4f08b4c to 75eb1ae Compare September 11, 2020 13:31

rraavi force-pushed the update-error-responses branch from 75eb1ae to 43cfb90 Compare September 11, 2020 18:49

Raghuveer Raavi added 2 commits September 25, 2020 20:31

Updated error response for 'tooManyConcurrencyRequests' to be more de…

412e80b

…scriptive

Fixed breaking test

6f27606

rraavi force-pushed the update-error-responses branch from 43cfb90 to 6f27606 Compare September 26, 2020 00:31

dgrove-oss reviewed Sep 29, 2020

View reviewed changes

rraavi mentioned this pull request Oct 2, 2020

Updated ActivationThrottler to compute limit and compare against totalInflightActivations in cluster rraavi/openwhisk#1

Open

mrutkows approved these changes Oct 7, 2020

View reviewed changes

style95 added the stale old issue which needs to validate label May 31, 2023

	s"Too many concurrent requests in flight (currently running: $count, allowed per controller: $allowed)."
	s"Too many concurrent requests (count in window: $count, allowed per controller: $allowed)."

Conversation

rraavi commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

rraavi commented Sep 9, 2020

Uh oh!

bdoyle0182 Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

rraavi Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

rraavi commented Sep 10, 2020

Uh oh!

mrutkows commented Sep 11, 2020

Uh oh!

bdoyle0182 commented Sep 15, 2020

Uh oh!

rraavi commented Sep 26, 2020

Uh oh!

dgrove-oss Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

dgrove-oss Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

rraavi Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

dgrove-oss Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

rabbah Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

rabbah commented Oct 7, 2020

Uh oh!

mrutkows left a comment

Choose a reason for hiding this comment

Uh oh!

rraavi commented Oct 9, 2020

Uh oh!

style95 commented Nov 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rraavi commented Sep 9, 2020 •

edited

Loading