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(gatsby-source-filesystem): add an environment variable to control concurrent queue size #13110

Conversation

violy
Copy link
Contributor

@violy violy commented Apr 4, 2019

related to #13073

@pieh
Copy link
Contributor

pieh commented Apr 4, 2019

Thanks for changes @violy!

Just curious what were values you used for timeout / retries - maybe better approach would be to change defaults there?

@violy
Copy link
Contributor Author

violy commented Apr 4, 2019

as I explain on the issue #13073 — during my troubleshooting, I observed this warning, for different reasons (server overload, network / local overload) related to my stack and my work environment, and my successive attempts to valid locally my build.

During my troubleshooting, I've try to change timeout & retries values, but with no effect. The only success was changing concurrent to lower value (10 instead of 200, in my case) the original question in my issue / PR.
But I think to other case where troubleshooting could be painful, access to const via .env variables or equivalent, is a good way to adjust the stack with no pain.

When I finally deployed my app, via netlify, there was no problem (no need to fix with custom fork 😅)!
So initial default static values may be not so bad!

@pieh
Copy link
Contributor

pieh commented Apr 4, 2019

So if changing those didn't have any effect I'm not sure it makes sense to add yet more moving parts. I would prefer to just add GATSBY_CONCURRENT_DOWNLOAD and more detailed error message here.

@muescha
Copy link
Contributor

muescha commented Apr 6, 2019

the new possible env variables should be anywhere documented:

  • GATSBY_CONCURRENT_DOWNLOAD
  • GATSBY_DOWNLOAD_TIMEOUT
  • GATSBY_DOWNLOAD_RETRIES

@violy violy force-pushed the issue-13073-createRemoteFileNode-concurrent-download branch from 7b09d17 to 2816051 Compare April 7, 2019 13:24
@violy
Copy link
Contributor Author

violy commented Apr 7, 2019

Thank you, according your comment @pieh
I've update my PR, by removing the variables GATSBY_DOWNLOAD_TIMEOUT & GATSBY_DOWNLOAD_RETRIES maybe useless.

@muescha
Copy link
Contributor

muescha commented Apr 7, 2019

Maybe it would be nice to have an "main entry point" for all "magic numbers" and a default solution how to overwrite it.

@LekoArts
Copy link
Contributor

@violy Hi 👋 Do you have time to update your PR (adding information to README) and resolve the conflicts? Would be cool to have this in!

@LekoArts LekoArts added the status: awaiting author response Additional information has been requested from the author label May 20, 2019
@violy violy force-pushed the issue-13073-createRemoteFileNode-concurrent-download branch from acb1bff to bbc975c Compare May 21, 2019 08:50
@violy violy changed the title [gatsby-source-filesystem] processRemoteNode concurrent requests overload feat(gatsby-source-filesystem) processRemoteNode concurrent requests overload May 21, 2019
@violy
Copy link
Contributor Author

violy commented May 21, 2019

@LekoArts thanks for your request,
so I've resolve conflict and add a small word in the README Configuration paragraph and force-push all the change in one commit.

@LekoArts LekoArts added status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response and removed status: awaiting author response Additional information has been requested from the author labels May 21, 2019
Copy link
Contributor

@antoinerousseau antoinerousseau left a comment

Choose a reason for hiding this comment

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

LGTM

@violy
Copy link
Contributor Author

violy commented May 29, 2019

Any news / reviewer ?

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

Longer term, we very much prefer to bake in optimized, smart defaults (or a system that adapts, e.g. will back off the queue if failures arise) but until #10049 lands this is an OK approach that we can offer to people who are blocked in some way.

Thank you!

@DSchau DSchau changed the title feat(gatsby-source-filesystem) processRemoteNode concurrent requests overload feat(gatsby-source-filesystem): add an environment variable to control concurrent queue size May 31, 2019
@DSchau DSchau merged commit 90aa247 into gatsbyjs:master May 31, 2019
@muescha
Copy link
Contributor

muescha commented May 31, 2019

This download manager should also include an cache and some strategies how not redownload media assets when nothing is changed.

@DSchau
Copy link
Contributor

DSchau commented May 31, 2019

@muescha I think the issue (#10049) is a better place for that comment; thanks! Otherwise it's very possible we'll lose track of your suggestions.

@muescha
Copy link
Contributor

muescha commented May 31, 2019

Is there also a stragegy only to download the used media assets (when maybe more assets are saved in wordpress but only some used in the used articles)?

@muescha
Copy link
Contributor

muescha commented May 31, 2019

Thanks for your hint. I was thinking i am already in the other issue. #commentOnMobileProblems ;)

@KyleAMathews
Copy link
Contributor

Yeah, env variables seem appropriate now as a temporary fix until we can work on #10049

mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants