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.

Sign up for GitHub

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

Jump to bottom

fix(gatsby-source-contentful): fixed contentful asset download stalling with high number of assets #27563

Merged
merged 4 commits into from
Oct 21, 2020
Merged

fix(gatsby-source-contentful): fixed contentful asset download stalling with high number of assets #27563

merged 4 commits into from
Oct 21, 2020

Conversation

me4502
Copy link
Contributor

@me4502 me4502 commented Oct 20, 2020

Description

With large Contentful sites, the number of assets can open too many connections for Node to handle. This leads to the download stalling. While the issue is unlikely to be encountered on a CI instance, on a local machine it can freeze downloads or even crash the machine in some cases. This PR adds a workload distribution system that limits the number of concurrent downloads, fixing the issue. The number is also configurable using a plugin option.

Documentation

Docs in the readme are updated.

Related Issues

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 20, 2020
pvdz
pvdz previously approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

I really like this approach. And you're right, we needed this throttle.

Thank you! 💜

@pvdz
Copy link
Contributor

pvdz commented Oct 20, 2020

Oh, well some tests need to be updated I guess.

@pvdz pvdz added topic: source-contentful Related to Gatsby's integration with Contentful topic: performance Related to runtime & build performance topic: scaling builds and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Oct 20, 2020
@pvdz pvdz dismissed their stale review October 20, 2020 10:51

tests failing

@me4502 me4502 requested a review from pvdz October 21, 2020 05:42
@me4502 me4502 force-pushed the fix/batched-contentful-assets branch from b13089a to 9ebd4d0 Compare October 21, 2020 05:43
pvdz
pvdz approved these changes Oct 21, 2020
View reviewed changes
Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you 💜

@pvdz pvdz merged commit dc0ce0c into gatsbyjs:master Oct 21, 2020
@pvdz
Copy link
Contributor

pvdz commented Oct 21, 2020

Published in [email protected] (same time as [email protected] although this is a standalone fix)

@pvdz
Copy link
Contributor

pvdz commented Oct 21, 2020

@me4502 this seems to be "breaking" the progress bar of the reporter because now it's printing one 1/1 line per image downloaded. Also appears to be doing it one by one, rather than in pools of 50. Are you seeing this as well?

@me4502
Copy link
Contributor Author

me4502 commented Oct 21, 2020

I'll take a look tomorrow (Australia time), there's a chance some of the minor cleanups I did at the end may have modified behaviour - but it's working fine on my local patch.

It does increase the number over time as it hasn't yet triggered the remote file download yet, but in my testing it was always faster to increase the number than it was to download, so it never triggered multiple messages (unless it was set to a small number such as 2)

@pvdz
Copy link
Contributor

pvdz commented Oct 21, 2020

I got it. The count needs to have a default in this function as well because it's receiving undefined, causing the array to schedule just one worker :) I'll fix it.

@me4502
Copy link
Contributor Author

me4502 commented Oct 21, 2020

Ah that explains it - I removed that when I added the config value as I assumed Joi would handle it. Thanks :)

@pvdz pvdz mentioned this pull request Oct 21, 2020
Merged
@muescha muescha mentioned this pull request Jan 7, 2021
Merged
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
@gatsbybot
fix(gatsby-source-contentful): fixed contentful asset download stalli…
ca91eb6 
…ng with high number of assets (gatsbyjs#27563)

* fix(gatsby-source-contentful): fixed contentful asset download stalling with high number of assets

* Add new option to README

* Fix test

* Switch to pop and fix a typo made when cleaning up code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvdz pvdz pvdz approved these changes

Assignees
No one assigned
Labels
topic: performance Related to runtime & build performance topic: source-contentful Related to Gatsby's integration with Contentful
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@me4502 @pvdz