Dark Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

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

Open
rraavi wants to merge 2 commits intoapache:masterfrom
rraavi:update-error-responses
Open

Updated error responses around rate limiting to be more descriptive#4972
rraavi wants to merge 2 commits intoapache:masterfrom
rraavi:update-error-responses

Conversation

Copy link

rraavi commented Sep 9, 2020 *
edited
Loading

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

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

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.

Copy link
Author

rraavi commented Sep 9, 2020

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

rraavi marked this pull request as draft September 9, 2020 16:09
bdoyle0182 requested changes Sep 9, 2020
/** 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)."
Copy link
Contributor

bdoyle0182 Sep 9, 2020

Choose a reason for hiding this comment

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

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

rraavi reacted with thumbs up emoji
Copy link
Author

rraavi Sep 10, 2020

Choose a reason for hiding this comment

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

Reverted in dc3ec0f

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
Copy link
Author

rraavi commented Sep 10, 2020

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

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

mrutkows commented Sep 11, 2020

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

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

bdoyle0182 commented Sep 15, 2020

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.

Raghuveer Raavi added 2 commits September 25, 2020 20:31
rraavi force-pushed the update-error-responses branch from 43cfb90 to 6f27606 Compare September 26, 2020 00:31
Copy link
Author

rraavi commented Sep 26, 2020

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 reviewed Sep 29, 2020
/** 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

dgrove-oss Sep 29, 2020

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

dgrove-oss Sep 29, 2020

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

rraavi Oct 2, 2020

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

dgrove-oss Oct 2, 2020

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

rabbah Oct 7, 2020

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

rabbah commented Oct 7, 2020

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 approved these changes Oct 7, 2020
Copy link
Contributor

mrutkows left a comment

Choose a reason for hiding this comment

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

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

mcdan reacted with thumbs up emoji
Copy link
Author

rraavi commented Oct 9, 2020

So, close the linked PR right?

Copy link
Member

style95 commented Nov 14, 2020

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

style95 added the stale old issue which needs to validate label May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

rabbah rabbah left review comments

dgrove-oss dgrove-oss left review comments

bdoyle0182 bdoyle0182 requested changes

mrutkows mrutkows approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

stale old issue which needs to validate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants