impl(auth): add retries for universe_domain endpoint on MDS client#5310
impl(auth): add retries for universe_domain endpoint on MDS client#5310alvarowolfx merged 12 commits intogoogleapis:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5310 +/- ##
=======================================
Coverage 97.57% 97.57%
=======================================
Files 222 222
Lines 47320 47419 +99
=======================================
+ Hits 46172 46271 +99
Misses 1148 1148 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Split |
dbolduc
left a comment
dbolduc
left a comment
There was a problem hiding this comment.
just nits, which I think are improvements, but you can decide if they are worth your time or not.
| backoff_policy: ExponentialBackoff::default().into(), | ||
| retry_throttler: Arc::new(Mutex::new(AdaptiveThrottler::default())), |
There was a problem hiding this comment.
nit: we are allocating these on every call to send(). We only need to do it once per MDS client.
There was a problem hiding this comment.
But we would need multiple MDS Clients on the MDS Credential, because some calls (access_token, id_token) should not have retries and universe_domain needs retries. That's why the retry is allocated per call.
There was a problem hiding this comment.
Why can't you store the backoff_policy and retry_throttler on the client and clone that for each send()? You are currently making new ones for every send() call, right? Am I missing something?
There was a problem hiding this comment.
I think I got what you're saying, we are cloning Client anyway, so would make sense for the retry to live in the Client and be cloned from it and self contained, since the send() methods belongs to Client
2 participants
Towards #3646