Skip to content

503 throttle graphql requests#512

Merged
marshmallowrobot merged 7 commits intomainfrom
503-throttle-graphql-requests
Nov 21, 2022
Merged

503 throttle graphql requests#512
marshmallowrobot merged 7 commits intomainfrom
503-throttle-graphql-requests

Conversation

@marshmallowrobot
Copy link
Copy Markdown
Collaborator

@marshmallowrobot marshmallowrobot commented Nov 15, 2022

This PR introduces the throttling plugin to our GraphQL calls. It prevents the rate-limiting errors we were seeing prior to this, but at the cost of increased time. It now takes a couple of minutes for scripts/generate_learning_path_markdown.js to complete.

The throttling plugin requires two new functions to be defined: onRateLimit() and onSecondaryRateLimit(), neither of which should get triggered if the throttling is working as intended. I didn't do anything different from the documentation in those.

I also made small changes to the auth and query objects. The @octokit README specifically mentions string template literals as an anti-pattern, so I removed them, and in the query, used a variable instead.

@rrrutledge
Copy link
Copy Markdown
Contributor

This work is amazing, @marshmallowrobot ❗️ I added @tsadler1988 to this pull request, who is the original author of a lot of this code. I’m excited to take a look, too!

Copy link
Copy Markdown
Contributor

@rrrutledge rrrutledge left a comment

Choose a reason for hiding this comment

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

Thank you for this work, @marshmallowrobot ‼️

Comment thread scripts/generate_learning_path_markdown.js
Comment thread scripts/get_contributors.js Outdated
Copy link
Copy Markdown
Contributor

@rrrutledge rrrutledge left a comment

Choose a reason for hiding this comment

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

Great stuff! So does the build actually finish now?

Comment thread scripts/generate_learning_path_markdown.js
Comment thread scripts/get_contributors.js Outdated
@marshmallowrobot
Copy link
Copy Markdown
Collaborator Author

Great stuff! So does the build actually finish now?

Locally, YES! It does. Takes 2.5-3 min to finish, but its consistent, and no rate limit responses from GitHub API

@lenucksi
Copy link
Copy Markdown
Member

Great stuff! So does the build actually finish now?

Locally, YES! It does. Takes 2.5-3 min to finish, but its consistent, and no rate limit responses from GitHub API

Sounds great, how about we try it?
I still remember a 10 minute pre-commit script, (not CI, just pre-commit) so 2.5-3 minutes sounds good to me. (Not that one should compare themselves to that pre-commit script. Company was fine, effect of the script was also useful, still too long. 😉 )

@marshmallowrobot
Copy link
Copy Markdown
Collaborator Author

Sounds great, how about we try it?

Ok! What do I do to try it? If I merge the PR, does that magically kick something off? Or do I have to go run another script somewhere? What's this process like?

@lenucksi
Copy link
Copy Markdown
Member

Sounds great, how about we try it?

Ok! What do I do to try it? If I merge the PR, does that magically kick something off? Or do I have to go run another script somewhere? What's this process like?

From what I can see here: https://github.com/InnerSourceCommons/InnerSourceLearningPath/blob/main/.github/workflows/publish-to-website.yml it should run. However, it appears to only run if certain directories have been touched by a commit. So might need a manual run.

@marshmallowrobot marshmallowrobot merged commit 6416019 into main Nov 21, 2022
@marshmallowrobot
Copy link
Copy Markdown
Collaborator Author

@lenucksi it worrrrrrked! like maaaagic! TY!

And TY! @rrrutledge for the timely PR review and approval ^_^

@rrrutledge
Copy link
Copy Markdown
Contributor

Woah! So amazing? So the website is updated now?

@rrrutledge rrrutledge deleted the 503-throttle-graphql-requests branch November 22, 2022 04:05
@rrrutledge
Copy link
Copy Markdown
Contributor

I see it! I see it! Wow‼️

@lenucksi
Copy link
Copy Markdown
Member

@lenucksi it worrrrrrked! like maaaagic! TY!

And TY! @rrrutledge for the timely PR review and approval ^_^

Boom, magic! Amazing stuff 😀 🚀

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.

Build failing (rate limited)

4 participants