Skip to content

Rate limit RISC ruby workers by destination (LG-4973)#5335

Merged
zachmargolis merged 5 commits intomainfrom
margolis-rate-limit-risc
Aug 26, 2021
Merged

Rate limit RISC ruby workers by destination (LG-4973)#5335
zachmargolis merged 5 commits intomainfrom
margolis-rate-limit-risc

Conversation

@zachmargolis
Copy link
Contributor

Builds on #5333

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok so, the attempts: :unlimited is a little spicy, but my thinking is, we want to do our best to deliver these notifications, and rate limiting is us throttling ourselves, and not even giving them an attempt to be down? So I think our limit would keep us to a fixed throughput, which is constant load for us but maybe not so bad?

Open to feedback on this

Screen Shot 2021-08-25 at 12 23 26 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well with the exponential delay, I think that the red line would go down more over time as the requests get spaced out more but same same

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have a maximum. We don't have visibility into why we aren't able to complete the request, and it creates a growing queue that limits the potentially successful events from going out. I'd rather start on the conservative side of "best effort" and improve what our "best" is as we go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, how does a maximum of 10 sound?

Copy link
Contributor

Choose a reason for hiding this comment

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

if it's 2^10 seconds (~17 minutes) that sounds good to me 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in 6138913

Base automatically changed from margolis-risc-ruby-worker to main August 26, 2021 19:24
@zachmargolis zachmargolis force-pushed the margolis-rate-limit-risc branch from 27fa159 to a8e7dcc Compare August 26, 2021 20:34
- Also make timeout retries count explicit
@zachmargolis zachmargolis merged commit ac88f62 into main Aug 26, 2021
@zachmargolis zachmargolis deleted the margolis-rate-limit-risc branch August 26, 2021 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants