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

feat(pullsync): rate limit handler #4799

Merged
merged 12 commits into from
Sep 9, 2024
Merged

feat(pullsync): rate limit handler #4799

merged 12 commits into from
Sep 9, 2024

Conversation

istae
Copy link
Member

@istae istae commented Sep 2, 2024

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

Added a basic rate limiter to pullsync handler.

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

@istae istae added the on hold Temporarily halted by other development label Sep 2, 2024
@istae istae marked this pull request as draft September 2, 2024 16:23
@istae istae marked this pull request as ready for review September 2, 2024 23:31
Copy link
Collaborator

@ldeffenb ldeffenb left a comment

Choose a reason for hiding this comment

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

Hard to tell if handler() has actually changed since it was relocated in the file, but aside from that and my two comments, I have no issues with this. Looking forward to seeing if it actually works!

It'd be nice to see metrics on how many delays actually occur, and/or how long the delay might be, but given the package layering, I don't see a reasonable way to do this.

pkg/puller/puller.go Outdated Show resolved Hide resolved
pkg/ratelimit/ratelimit.go Outdated Show resolved Hide resolved
@istae istae removed the on hold Temporarily halted by other development label Sep 8, 2024
@istae istae merged commit 2d75623 into master Sep 9, 2024
26 checks passed
@istae istae deleted the pullsync-limiter branch September 9, 2024 11:06
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.

4 participants