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: improve request throttling [ZEND-4197] #1255

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

marcolink
Copy link
Member

@marcolink marcolink commented Oct 11, 2023

We currently don't have a good way of throttling big amount of requests. For other tools we already use pQueue or pThrottle to limit the number of requests/s. This is mostly relevant for migration with transform functions on entries, as the amount of affected entities can easily be > 1000.

This changes introduces a new CLI flag request-limit (defaults to 10, as this is the smallest limit from our free offering) which describes the max amount of requests this tool initiates within a second.

@marcolink marcolink marked this pull request as ready for review October 12, 2023 14:51
@marcolink marcolink requested a review from a team as a code owner October 12, 2023 14:51
Copy link
Contributor

@ruderngespra ruderngespra left a comment

Choose a reason for hiding this comment

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

Nice.

We should expose this flag in contentful-cli then as well I guess.

@@ -56,11 +70,14 @@ export const createMakeRequest = (client: PlainClientAPI, { spaceId, environment
const { url, ...config } = requestConfig
const fullUrl = makeBaseUrl(url)

return client.raw.http(fullUrl, config)
return throttle(() => client.raw.http(fullUrl, config))()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I get this correctly that createMakeRequest is the only constructor in this project that creates functions which trigger API requests (makeRequest instances), and by wrapping the throttle here we effectively throttle all requests, both for the fetching in preparation of the migration as well as for the applying of the changes in the migration, because wherever api interactions happen they do with makeRequest?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly!

@marcolink
Copy link
Member Author

Nice.

We should expose this flag in contentful-cli then as well I guess.

Would like to first get a customer confirmation that it all works as expected

@marcolink marcolink force-pushed the feat/improve-request-throttling branch from 89f0355 to c493ec7 Compare October 19, 2023 16:30
@marcolink marcolink merged commit 2c08081 into master Oct 19, 2023
2 checks passed
@marcolink marcolink deleted the feat/improve-request-throttling branch October 19, 2023 16:34
@contentful-automation
Copy link

🎉 This PR is included in version 4.17.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants