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

Enable server rate limit #1772

Merged
merged 2 commits into from Mar 25, 2024
Merged

Enable server rate limit #1772

merged 2 commits into from Mar 25, 2024

Conversation

TBonnin
Copy link
Collaborator

@TBonnin TBonnin commented Mar 1, 2024

Describe your changes

Enabling rate-limiting (was only logging so far).
Since I pushed the memoization of getConnection yesterday, no rate limiting has been logged so it should not impact anybody.

followup of #1708

Copy link
Member

@khaliqgant khaliqgant left a comment

Choose a reason for hiding this comment

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

Should we give this more time? For some customers syncs run just once a week so are we really be sure that this won't affect anyone? I would suggest waiting or at least announce it

Copy link
Contributor

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

Let's go 👍🏻
Should we document that somewhere? @bastienbeurier

@TBonnin
Copy link
Collaborator Author

TBonnin commented Mar 1, 2024

Should we give this more time? For some customers syncs run just once a week so are we really be sure that this won't affect anyone? I would suggest waiting or at least announce it

Sure. we can wait. @bastienbeurier what do you think.
wrt the doc I created a ticket https://linear.app/nango/issue/NAN-486/document-rate-limit

@bastienbeurier
Copy link
Member

Thanks for pointing out @bodinsamuel and for creating a ticket @TBonnin. I'll take care of it since I'm deep in the docs these days.

@TBonnin
Copy link
Collaborator Author

TBonnin commented Mar 4, 2024

@khaliqgant after how long would you feel confident to merge/release this PR?

@khaliqgant
Copy link
Member

@khaliqgant after how long would you feel confident to merge/release this PR?

I would say a week so we can get a picture, so I guess that would be Thursday?

@TBonnin TBonnin enabled auto-merge (squash) March 25, 2024 12:23
@TBonnin TBonnin merged commit ba21794 into master Mar 25, 2024
18 checks passed
@TBonnin TBonnin deleted the tbonnin/enable-rate-limit branch March 25, 2024 12:26
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.

None yet

4 participants