-
Notifications
You must be signed in to change notification settings - Fork 5
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
Retry on http status 429, 500, 503? #132
Comments
good idea @dobesv - wanna work on that yourself maybe? :) |
@dobesv definitely something we can add. I would be avoiding additional dependencies, esp on axios, as I have plans to move to node native fetch and moving to modules. |
Couldn't replicate this so far. @dobesv do you have an activity im mind that triggers this regularly for you? |
I don't have any great ideas how to test it. You would have to send a high volume of requests to the API until it rate limits you, to get this in a live environment, which could be a bit of a pain. Maybe there's a way to simulate a response using some kind of mock adapter or something? |
Hey @dobesv, Yea i tried 20k requests over 5 min and still didnt git anything using soap or rest. Given Marketing Cloud's ability to document one thing and implement another or implement differently depending on the request, unless we can get a real response, I wouldn't be a fan of adding this in. We do have mocks in the tests we run, so once we have a concrete example, we could then add that in, but that example is currently missing from the wild it seems :( have you ever seen it? |
Sure, if it's a pain to add feel free to punt on it. I don't recall actually getting throttled I just thought it would be nice to put measures in place "just in case". But I wouldn't spend a ton of time writing your own custom retry system, I thought maybe there would be a library you could use that would handle this easily. |
If this starts causing us some pain I can try to open a PR later, or maybe someone else will step in and fix it. For anyone who does try and take it on, if you use |
Another way to handle this that could have some beneficial side-effects is to allow users of the library to provide their own axios instance. In addition to allowing us to add our own retry logic, it would also allow us to provide a mock axios instance for our own tests, add proxy options, stuff like that. |
@dobesv indeed, without being certain of people getting it I would leave it for now. As I mentioned, the docs often don't align 1:1 with reality so wouldn't want to overdesign it and then have it not work. |
It might be nice if the library would retry on http status 429, 500, 503 e.g. using
axios-retry
. If theRetry-After
header is provided, it should honor that.e.g. see https://developer.salesforce.com/docs/marketing/marketing-cloud/guide/rate-limiting-best-practices.html
The text was updated successfully, but these errors were encountered: