Skip to content

Updated error responses around rate limiting to be more descriptive #4972

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Updated error responses around rate limiting to be more descriptive #4972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
412e80b
Updated error response for 'tooManyConcurrencyRequests' to be more de…
Sep 9, 2020
6f27606
Fixed breaking test
Sep 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ object Messages {

/** 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)."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest a change to address @bdoyle0182 's comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.


/** System overload message. */
val systemOverloaded = "System is overloaded, try again later."
Expand Down
2 changes: 1 addition & 1 deletion tests/src/test/scala/limits/ThrottleTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ class NamespaceSpecificThrottleTests
}

wsk.action.invoke(actionName, expectedExitCode = TooManyRequests.intValue).stderr should {
include(prefix(tooManyConcurrentRequests(0, 0))) and include("allowed: 0")
include(prefix(tooManyConcurrentRequests(0, 0))) and include("allowed per controller: 0")
}
}

Expand Down