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

PERSONA-180 run rate limit when there is a response #127

Conversation

decarvalhorobinson
Copy link
Contributor

When an error occurs the client will not run the rate limit, which causes too many requests to be fired and then will hit the endpoints rate limit. So to avoid this, the rate limit function will be run when there is a response from the client payload. This was noticed when running terraform provider with invalid fields, which caused 400 errors

@decarvalhorobinson decarvalhorobinson changed the title PERSONA-180 run time limit when there is a response PERSONA-180 run rate limit when there is a response May 20, 2021
Copy link
Contributor

@mburtless mburtless left a comment

Choose a reason for hiding this comment

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

Such a simple solution, I love this.

Copy link
Contributor

@mburtless mburtless left a comment

Choose a reason for hiding this comment

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

Actually, one question on this

rest/client.go Outdated
@@ -164,6 +164,12 @@ func SetDDIAPI() func(*Client) {
// non-2XX response.
func (c Client) Do(req *http.Request, v interface{}) (*http.Response, error) {
resp, err := c.httpClient.Do(req)

if resp != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this check below the if err != nil beneath this? I don't think there's a reason to eschew Go's convention for error checking immediately after the func call, since HTTP error status code does not result in httpClient.Do returning an err, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the "doer" doesn't return any errors for the requests that have responses, it makes sense to move it down after the first error checking. I am testing, if everything works well I will make another commit moving it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved since it seems what you said is correct, so it makes senses to keep it after the first error checking

@decarvalhorobinson decarvalhorobinson force-pushed the PERSONA-180/check-rate-limit-before-returning branch from 8427723 to 571c6b1 Compare May 20, 2021 18:26
@decarvalhorobinson decarvalhorobinson force-pushed the PERSONA-180/check-rate-limit-before-returning branch from 571c6b1 to e610b7b Compare May 20, 2021 18:26
Copy link
Contributor

@mburtless mburtless left a comment

Choose a reason for hiding this comment

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

🔥

@mburtless mburtless merged commit 8b6a4f2 into ns1:v2 May 20, 2021
@decarvalhorobinson decarvalhorobinson deleted the PERSONA-180/check-rate-limit-before-returning branch May 20, 2021 19:22
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.

3 participants