[GraphQL-Client][Feature]Add the retries feature to the client #1017
Conversation
| await sleep(resetTime); | ||
| return httpFetch(params, nextCount, maxTries); | ||
| } | ||
|
|
There was a problem hiding this comment.
Note: This error only occurs when the request was abandoned (eg. no network).
If the server has responded, the client will return the last server response (as per the previous block).
melissaluu
changed the title
melissaluu
changed the title
melissaluu
changed the title
There was a problem hiding this comment.
Love it @melissaluu! We were only discussing this yesterday.
LGTM - will defer to @paulomarg for the ✅
|
|
||
| return response; | ||
| } catch (error) { | ||
| if (nextCount <= maxTries) { |
There was a problem hiding this comment.
I forget where we landed on logging - would it be worth letting them know when a retry occurs?
There was a problem hiding this comment.
I haven't implemented logging yet - that's my next task 😄
There was a problem hiding this comment.
You're a wizard. :TIL: x 100
melissaluu
force-pushed
the
ml-graphql-client-retries
branch
from
60caf87 to
171cb94
Compare
packages/graphql-client/README.md
Outdated
| | headers? | `{[key: string]: string}` | Additional and/or replacement headers to be used in the request | | ||
| | retries? | `number` | Alternative number of retries for the request. Retries only occur for requests that were abandoned or if the server responds with a `Too Many Request (429)` or `Service Unavailable (503)` response. | |
There was a problem hiding this comment.
Would it make sense to do this for any 5XX error? I guess we don't want to end up flooding the server with retries if there's an actual outage going on?
There was a problem hiding this comment.
Yeah - the last thing we want to do is to flood the server when there's an actual outage - let's stick with only retrying for 503s for now (since this is a status code that does support retries - it's 1 of 3 status' that can return a retry-after header) and then we can see if devs gives other feedback and we can reevaluate.
|
@paulomarg - do you think we should make an upper limit on how many retries a dev can set? The last thing we want is for someone to set a ridiculous # for retries 🤔 😨 I'm thinking 5 or 10? thoughts? |
Definitely a good call. I would even be a little stricter, I wouldn't go past 5 - this is meant to be a way to recover for certain niche cases like an idempotent job or some such, not a way to brute force requests until they work. |
@paulomarg - if we're good with being stricter, I'd go for max |
melissaluu
force-pushed
the
ml-graphql-client-retries
branch
from
a1e52ef to
efc66ea
Compare
melissaluu
changed the title
WHY are these changes introduced?
We are adding the
retriesfeature to thegraphql-clientto enable auto retrying of failed HTTP requests in both thefetchandrequestfunction. The HTTP request retries only occurs in the following conditions:429 - Too Many Request503 - Service UnavailableA
1sdelay is used between each network retries.Client users are able to set the number of retries in 2 ways:
retriesconfig field during client initialization - this value will be applied to all HTTP requests by the clientretriesvalue in theRequestOptionsconfig per requestBy default, the client's
retriesvalue is set to0if none was passed in the initialization configuration.WHAT is this pull request doing?
This PR adds the
retriessettings and functionalities to thegraphql-client.Type of change
Checklist
yarn changesetto create a draft changelog entry (do NOT update theCHANGELOG.mdfile manually)