Added optional rate limit handling.

[andy-trimble]

This addresses issue #100.

[SendGridDX]

All committers have signed the CLA.

[thinkingserious]

Sorry for the inconvenience @andy-trimble, but we need this CLA signed also. Unfortunately, the CLA does not propagate across all repos.

Also, could you please take care of the conflict? Thanks!

[tariq1890]

I thought I'll chime in here since I had volunteered to fix issue #100. These are the issues that I'd think you need to fix

i. You are using a constant retry time. I believe the expected way is to use you read the Rate limit Reset header to get the sleep time.
ii. I strongly think the method should be defined at the rest library level rather than here. Which is why I waiting for #94 to get closed out.
iii. Also, as per previous discussions, it was decided that users would be given an option to use this retry on limit feature. Shouldn't we be using a boolean flag for this?

[andy-trimble]

@tariq1890 Thanks for the comment.

1. Good point. That should be an easy fix.
2. I'd be interested in why you think it belongs in the rest library? Especially if we use the "X-RateLimit-Remaining" it's a SendGrid specific solution.
3. This implementation is optional. It is a separate function call. API() is synchronous, and Request() is asynchronous with a retry on rate limit.

[tariq1890]

Hi @andy-trimble

2. The rest library I am referring to is the sendgrid/rest repo. I believe that is the repo used to construct requests and process responses. The retry-on-limit falls within that scope IMO. Since it is a sendgrid repo, this sendgrid-specific task is pertinent IMO.

3.As an outsider who would use the API, I don't find the fact that API would mean synchronous and Request() would mean async obvious. I believe a flag would be better for the sake of readability here. Third-pary users would find that more intuitive no?

[andy-trimble]

@tariq1890

2. That particular Rest library is a SendGrid library, but it is a generic rest client. We happen to use it internally for things that won't behave this way. This would be onerous for systems that have a different rate limiting schema, for example.

3. I do agree that API() isn't a good function name 😄 . I didn't want to make that broad of a change (and thereby break backwards compatibility). I think it would fall to another issue to fix that. Do you have a suggestion for a function name aside from Request()?

4. If it is desirable to actually have a blocking rate limit aware call, then I can definitely make a separate function for that. But I do believe the asynchronous, channel-based solution provides merit.

[tariq1890]

2. Gotcha. That makes sense.

3. Can we consider adding this as an extra flag parameter to the method? The sendgrid-php repo that @thinkingserious referenced uses the flag iirc.

4. I was just going to suggest that. Block until the response is 200 OK for retry-on-limit. Right your code only retries once getting a 429 response.

[andy-trimble]

3. I'm open to this implementation, but I prefer a clean, boolean-less interface. I wouldn't mind so much if this were, say, Python or PHP and we could specify a default value. But generally, the fewer function parameters, the better (and more readable). Additionally, altering the API() function breaks backwards compatibility, something I was trying to avoid.

4. I'll add a blocking function.

[thinkingserious]

@suchitparikh

[thinkingserious]

@tariq1890, @andy-trimble,

Currently, @suchitparikh is working on refactoring this repo, per this issue. I encourage you to collaborate with him there.

With regards to the conversation in this thread:

3. In the rest client, we are renaming API to Send and passing through to the API function to avoid the breaking change. But, in @suchitparikh's refactor we are brining that functionality into the library.

[thinkingserious]

Hello @andy-trimble,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX