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

Distribute ratelimiter requests across the cluster by route #621

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

jchristgit
Copy link
Collaborator

As described in #620, currently the ratelimiter is not usable across
multiple nodes. Lay the groundwork for doing this by registering
ratelimiters in a process group and selecting the matching ratelimiter
based on the request route.

Note that some minor further adjustments need to be made to make the
ratelimiter fully functional over multiple nodes, primarily related to
process registration, but some further documentation is also missing and
will be amended once manual consumer startup is prepared.

@jchristgit jchristgit requested a review from jb3 August 10, 2024 15:31
@jchristgit jchristgit marked this pull request as ready for review August 10, 2024 15:31
`:erlang.phash2/2`.
"""

@scope_name __MODULE__
Copy link
Contributor

@Th3-M4jor Th3-M4jor Aug 10, 2024

Choose a reason for hiding this comment

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

Should we make this value configurable? Issues could theoretically arise if you were running multiple bots and had the nodes connected, you'd get 403s that were difficult to debug if you weren't aware of this module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should make not the scope name but the group name configurable to set it to the bot ID that the ratelimiters run for.

Would running multiple bots with distinct configurations even work currently? I think you would run into other confusing bugs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think for later we should make the group name configurable in the start_link function. For now I'm not sure we need to, because plenty of other things break. What do you think?

lib/nostrum/api/ratelimiter_group.ex Outdated Show resolved Hide resolved
As described in #620, currently the ratelimiter is not usable across
multiple nodes. Lay the groundwork for doing this by registering
ratelimiters in a process group and selecting the matching ratelimiter
based on the request route.

Note that some minor further adjustments need to be made to make the
ratelimiter fully functional over multiple nodes, primarily related to
process registration, but some further documentation is also missing and
will be amended once manual consumer startup is prepared.
@jchristgit jchristgit merged commit 41d421c into master Aug 16, 2024
9 of 10 checks passed
@jchristgit jchristgit deleted the ratelimiter-phash2 branch August 16, 2024 19:01
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.

3 participants