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

fix(gatsby-source-filesystem): Reduce default download concurrency to a saner value #28548

Closed
wants to merge 1 commit into from

Conversation

sidharthachatterjee
Copy link
Contributor

Under the hood, createRemoteFileNode uses a Queue to process network requests. The number of concurrent workers for this queue is controlled by the GATSBY_CONCURRENT_DOWNLOAD env variable.

However, when the variable is not set, this defaults to 200. 200 is a very high number of concurrent HTTP connections to open to pretty much any remote and such load sustained over a few seconds typically results in the remote rate limiting us and not accepting any other connections after 40-50 of them.

It also results in timeouts, incomplete downloads etc which all result in extra retries required for most files.

10 is a much saner default and ensures consistent, stable downloads for most remotes.

A user may still choose to bump this up and may use the GATSBY_CONCURRENT_DOWNLOAD env var to do that. We are only changing the default so users without a opt in value can have a better default.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 9, 2020
@sidharthachatterjee sidharthachatterjee removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 9, 2020
@mxstbr
Copy link
Contributor

mxstbr commented Dec 9, 2020

Integration test failures are unrelated, see #27781.

Have we done any profiling of how much of a performance hit or gain this is? 🤔 I'd be curious to see some real-world stats

@sidharthachatterjee
Copy link
Contributor Author

@mxstbr Excellent question! I did manage to get rate limited by Fastly very consistently when it was set to 200 and that's how I found the issue.

@wardpeet
Copy link
Contributor

wardpeet commented Dec 9, 2020

Can we not make this change but put a message when timeout happen in remote-file node to tell them to lower the env var?

@wardpeet
Copy link
Contributor

wardpeet commented Dec 9, 2020

It would be great to capture this with telemetry, see what the sweet spot is for most people

@sidharthachatterjee
Copy link
Contributor Author

Can we not make this change but put a message when timeout happen in remote-file node to tell them to lower the env var?

@wardpeet This makes sense but 200 in general feels like a really large value for concurrent connections? While we could warn, it's another thing users will have to notice and do to fix their builds. Changing the default will just fix it for users without any action required.

What do you think?

@mxstbr
Copy link
Contributor

mxstbr commented Dec 9, 2020

I'm in favor of using a more sensible default and then adding that log in a follow-up PR 👍

@wardpeet
Copy link
Contributor

wardpeet commented Dec 9, 2020

I disagree, let's put up a message and add it to telemetry. We don't know what positive or negative impact this have on sites. What would that value even be? 100? 50? 75? Without data, we shouldn't change anything.

@sidharthachatterjee
Copy link
Contributor Author

@wardpeet We might not have the perfect value. But do we agree that 200 is too high a value?

@LekoArts LekoArts closed this May 7, 2021
@LekoArts LekoArts deleted the fix/default-download-concurrency branch May 7, 2021 12: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