Skip to content
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

Multi-threaded Limitador #37

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

didierofrivia
Copy link
Contributor

No description provided.

@didierofrivia didierofrivia force-pushed the rfc/limitador-multithread-inmemory branch 2 times, most recently from 42d042a to 861612d Compare November 3, 2023 15:35
@didierofrivia didierofrivia force-pushed the rfc/limitador-multithread-inmemory branch from 861612d to dbbd516 Compare January 17, 2024 14:44
@didierofrivia didierofrivia force-pushed the rfc/limitador-multithread-inmemory branch from dbbd516 to 9ba163d Compare February 8, 2024 19:28
@didierofrivia didierofrivia marked this pull request as ready for review February 9, 2024 07:33
@didierofrivia didierofrivia requested review from alexsnaps and a team February 9, 2024 07:33
@didierofrivia didierofrivia force-pushed the rfc/limitador-multithread-inmemory branch from f2ca2d9 to af76b43 Compare February 9, 2024 07:35
@didierofrivia didierofrivia force-pushed the rfc/limitador-multithread-inmemory branch from 7ffaf67 to 3d16710 Compare February 14, 2024 16:49
Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

I might be wrong, but it seems to me that no one requires 100% accuracy. So --mode=accuracy would not be necessary. For me it is more about what level of the rate throughput / accuracy you want. Having valid values meaningful names, never numbers. Something like

  • high => I do not care about accuracy, I want it as fast as possible
  • medium => I care about both
  • low => I care about accuracy but assuming it will not be 100% accurate

# Motivation
[motivation]: #motivation

Currently, Limitador service is single threaded, regardless of its chosen storage. This means that it can only process one request at a time.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually not true, is it? The gRPC service (tonic) is built on top of Tokio, an event-driven, non-blocking I/O platform. Or when the storage is memory, there is no I/O involved, hence only one request at a time can be processed?

Copy link
Member

Choose a reason for hiding this comment

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

See this issues, other than something having changed in newer version, this was still true like 10 months ago.

value given per service.
* **Throughput**: The _throughput_ of a service is defined by the number of requests that the service can process in a
given time. As a consequence of having higher throughput, we need to introduce two more concepts:
* **Overshoot**: The _overshooting_ of a limit counter is defined by the difference between the _expected_ value of the counter
Copy link
Contributor

Choose a reason for hiding this comment

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

overshooting is when the counter goes beyond the limit and the request is still admitted, right?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I agree with your definition, but looks like @didierofrivia looks at it the other way around.

## Behaviour

When using the multi-threading approach, we need to understand the trade-offs that we are making. The main one is that
it's not possible to have both _Accuracy_ and _Throughput_ at the same time. This means that we need to choose one of
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice to explain why both are not achievable.

Now imagine that we have the following requests happening at the same time in parallel: `POST`, `POST`, `POST`, `GET`,
`GET`; it could happen that the service authorizes the first two `POST` requests and updates both counters to 2, then the third
`POST` request is not authorized *but* the counter of limit 1 is updated to 3 (wrongly), and finally then only one `GET` request
that comes in would be authorized, leaving one wrongly denied. In this case, we would have an _overshoot_ of 1 in the
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say this is a undershooting example, i.e. traffic is being rejected before reaching the limit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I may need to review my understanding about these concepts.

1. Retrieve the Counter object from the collection.
2. Compare the current timestamp with the expiration time of the counter to determine if the request falls within the
sliding window
3. If the request falls within the window, increment the hit count of the Counter.
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the increment happen only when it has been verified that no counters has exceeded the associated limit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready For Review
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants