INFRA-42849 | Add retries to token generation

[BhavikaSharma]

• Add retries to token generation
• Add exponential backoff on failures

[msuthar-splunk]

This is a Splunk specific escalation implementation. Should not be added to this readme.

[msuthar-splunk]

Aggressive retry would actually make this worse. The backoff might need to be a bit more gentle.
0, 3, 5, 10

[dmarquez-splunk]

small nitpick but in golang its typica to check if the error != nil and error out if true instead of checking if it is nil and putting the logic in the brackets

[msuthar-splunk]

+1

[BhavikaSharma]

Let me know if you guys think otherwise but we have to check that the err is nil. When the err is nil, we want to break out of the loop. If we only return failure when err is not nil, when there is no error, it will go through the loop again and do a retry even when we don't want to.

Happy to discuss further if you see an alternative.

[dmarquez-splunk]

couldn't you just add a break so that it doesn't continue loop again when no error?

[BhavikaSharma]

The if-statement to check the break condition would be on if err == nil though so we would still need to check for this condition. This solution would require us to do two checks (technically 3 since there's an && needed and this check should already be encompassed by the for loop condition). I.e.:

for retries < totalRetries {
 logic
  if err != nil && retries < totalRetries -1 {
    return error
  } else if err == nil {
    break
  }
logic
}

Happy to implement it this way if you still think it's better but if we're checking for if err == nil anyway in this solution, and also since the for loop is already handling the retries < totalRetries seems like it makes sense to not make the changes. Let me know if you think otherwise.

[skan-splunk]

Yes, in this case I think

err == nil {
  //early exit
}

is cleaner than:

err != nil {
  continue
} else {
  //exit
}

I think the loop could look cleaner if we used for retries := range(retryWaitSeconds). Avoids having to increment

[skan-splunk]

Do we need to have a separate variable for number of totalRetries, will this always be the same as range(retryWaitSeconds)?