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

Disable RateLimitTransport #845

Closed
lijok opened this issue Jul 2, 2021 · 6 comments · Fixed by #907
Closed

Disable RateLimitTransport #845

lijok opened this issue Jul 2, 2021 · 6 comments · Fixed by #907
Labels
Provider Type: Feature New feature or request
Milestone

Comments

@lijok
Copy link

lijok commented Jul 2, 2021

Either completely disable it or at least give people the option to disable it through config
Imported some 3000 github resources into terraform yesterday, and then spent the better half of today trying to figure out why my terraform runs started taking 15-20 minutes
Commented this line out https://github.com/integrations/terraform-provider-github/blob/master/github/config.go#L34, and runs went down to 20-30 seconds with parallelism of 25
Increasing parallelism above that triggers github api abuse mechanism
The PR that introduced this mechanism #145 acknowledges the slowdown but shrugs the reduced chance of triggering the abuse mechanism a win
Then someone raised a follow-up pr giving users the option to disable this transport, which didn't go anywhere. #235
Please reconsider your stance on this. A 45x slowdown is unacceptable.

Thank you

@jcudit jcudit added Type: Feature New feature or request Provider labels Jul 13, 2021
@jcudit
Copy link
Contributor

jcudit commented Jul 13, 2021

give people the option to disable it through config

I like this direction as a first step. Ideally we implement this and keep 👀 out for reported improvements.

@lijok
Copy link
Author

lijok commented Aug 25, 2021

Is anything being done about this?
At the very least this: #849 should be addressed to alieviate some of the pressure
we have around 2k~ resources in our state and plans are taking 16min
image

@jcudit jcudit added this to the v4.15.0 milestone Aug 31, 2021
@jcudit
Copy link
Contributor

jcudit commented Aug 31, 2021

Aiming to address this in one of our upcoming releases. Adding provider-level configuration to disable RateLimitTransport functionality seems within reach.

@lijok
Copy link
Author

lijok commented Aug 31, 2021

Aiming to address this in one of our upcoming releases. Adding provider-level configuration to disable RateLimitTransport functionality seems within reach.

Thank you, greatly appreciated

@jcudit
Copy link
Contributor

jcudit commented Sep 7, 2021

We scoped this out and recommend the following edits:

  • Modify github/provider.go and index.md to add a new configuration option controlling the number of milliseconds the provider will sleep between attempts when it detects rate limiting.
  • Track this value in the Config struct assembled during providerConfigure() in provider.go.
  • Add convenience functions to transport.go to support configuring a NewRateLimitTransport with a desired delay
  • Add testing similar to TestRateLimitTransport_abuseLimit_post.

This looks on track for an upcoming release.

@jcudit jcudit modified the milestones: v4.15.0, v4.16.0 Sep 7, 2021
@lijok
Copy link
Author

lijok commented Sep 8, 2021

We scoped this out and recommend the following edits:

* Modify `github/provider.go` and `index.md` to add a new configuration option controlling the number of milliseconds the provider will sleep between attempts when it detects rate limiting.

* Track this value in the `Config` struct assembled during `providerConfigure()` in `provider.go`.

* Add convenience functions to `transport.go` to support configuring a `NewRateLimitTransport` with a desired delay

* Add testing similar to `TestRateLimitTransport_abuseLimit_post`.

This looks on track for an upcoming release.

Thank you, that sounds great
I'd also like to bring this issue to your attention specifically, as it is closely related to the slowness of the provider:
#849
As far as I understand, githubs official guidelines don't account for this issue?
Would it be reasonable to say that something needs to be done to ensure the provider transport does not sleep between each Query, sleeping inbetween Mutations instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Provider Type: Feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants