Adding retries to most common location service calls #10931
Conversation
shubham90
requested review from
damccorm,
jotaylo,
madhurig,
sachinma and
zjrunner
as code owners
| try { | ||
| return await action(); | ||
| } catch (error) { | ||
| tl.debug(`Network call failed. Number of retries left: ${maxRetries}`); |
There was a problem hiding this comment.
what would be in the "error" here. anything useful to log for the intermediate retries?
There was a problem hiding this comment.
It's mostly empty json in the cases I have seen. We are logging the last error in the caller method. Do you still want to log every error from retries?
There was a problem hiding this comment.
if it isn't structured, then i agree better not to log. it would be nice if high level result was logged. status code (or timeout), afdheader if applicable, etc. but probably need to make a change in vss-node-api to do that.
There was a problem hiding this comment.
so long as the final result at least looks as good as it does today (it says ECONNRESET, or ETIMEDOUT, etc)
There was a problem hiding this comment.
it does, final result is same/better than what it is today.
| @@ -94,7 +94,7 @@ export async function getPackagingUris(protocolType: ProtocolType): Promise<Pack | |||
|
|
|||
| tl.debug('Acquiring Packaging endpoints from ' + serviceUri); | |||
|
|
|||
| const connectionData = await locationApi.getConnectionData(interfaces.ConnectOptions.IncludeServices); | |||
| const connectionData = await retryOnExceptionHelper(() => locationApi.getConnectionData(interfaces.ConnectOptions.IncludeServices), 5, 1000); | |||
There was a problem hiding this comment.
i thought that the node api already does retries under the covers (for known status codes). For these known case does this mean we will now retry 5x5 times (assuming 5 is what is under covers?). If so, maybe we should be less aggressive about these wrapping retries (3 total calls?)
There was a problem hiding this comment.
Yes, that's correct. I was fighting my mind between 3 and 5. Now that you've said 3, 3 it is.
| @@ -37,7 +37,7 @@ export async function getServiceUriFromAreaId(serviceUri: string, accessToken: s | |||
|
|
|||
| tl.debug(`Getting URI for area ID ${areaId} from ${serviceUri}`); | |||
| try { | |||
| const serviceUriFromArea = await locationApi.getResourceArea(areaId); | |||
| const serviceUriFromArea = await retryOnExceptionHelper(() => locationApi.getResourceArea(areaId), 5, 1000); | |||
There was a problem hiding this comment.
is this code used by the nuget authenticate task?
There was a problem hiding this comment.
or is there a separate place to update for that?
There was a problem hiding this comment.
Not the new one, I am first going to first verify this and then port the code over there.
| @@ -150,6 +150,27 @@ export function getWebApiWithProxy(serviceUri: string, accessToken?: string): vs | |||
| return new vsts.WebApi(serviceUri, credentialHandler, options); | |||
| } | |||
|
|
|||
| // This function is to apply retries generically for any unreliable network calls | |||
| async function retryOnExceptionHelper<T>(action: () => Promise<T>, maxRetries: number, retryIntervalInMilliseconds: number): Promise<T> { | |||
There was a problem hiding this comment.
isn't this maxTries not maxRetries? because you first decrement the variable before checking < 1.
Adding retry to two most common location service calls that shouldn't fail ever on hosted but sometimes does fail because of timeouts: