impl(auth): add retries for universe_domain endpoint on MDS client#5310
impl(auth): add retries for universe_domain endpoint on MDS client#5310alvarowolfx wants to merge 1 commit intogoogleapis:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5310 +/- ##
==========================================
- Coverage 97.78% 97.76% -0.03%
==========================================
Files 220 220
Lines 45833 45975 +142
==========================================
+ Hits 44818 44947 +129
- Misses 1015 1028 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if !retry_config.has_retry_config() { | ||
| return self.send(request, error_message).await; | ||
| } |
There was a problem hiding this comment.
nit: This doesn't seem like a special case. I think we should always use a retry loop, and the default settings are policies that don't retry. That is less complexity IMO.
| let req = request | ||
| .try_clone() | ||
| .expect("client libraries only create builders where `try_clone()` succeeds"); | ||
| let response = req | ||
| .send() | ||
| .await | ||
| .map_err(google_cloud_gax::error::Error::io)?; | ||
|
|
||
| let status = response.status(); | ||
| if !status.is_success() { | ||
| let err_headers = response.headers().clone(); | ||
| let err_payload = response.bytes().await.map_err(|e| { | ||
| google_cloud_gax::error::Error::transport(err_headers.clone(), e) | ||
| })?; | ||
| return Err(google_cloud_gax::error::Error::http( | ||
| status.as_u16(), | ||
| err_headers, | ||
| err_payload, | ||
| )); | ||
| } |
There was a problem hiding this comment.
why are we duping the send(...) code? can we reuse it?
There was a problem hiding this comment.
send(...) maps errors to Credentials Errors and is simpler (we don't need to clone requests). On the retry loop we need to map to gax errors and clone requests. I tried to reuse both, but code was not looking nicer. We could make send(...) already return gax errors and map later to credentials errors, but we can loose some information as gax errors are more specific
| pub(crate) fn universe_domain(&self) -> UniverseDomainRequest { | ||
| UniverseDomainRequest { | ||
| client: self.clone(), | ||
| retry_config: RetryConfig::default(), |
There was a problem hiding this comment.
what about creating the policies once, and storing them on the client?
There was a problem hiding this comment.
we want to have different policies per call, because the MDS Client is reused on the MDS Credentials provider. For calls to fetch tokens we don't want retries, because the TokenCache already handles retries. We only want retries here for universe_domain calls
There was a problem hiding this comment.
What I want is to allocate the policies once.
If we only make one attempt to get the universe_domain per credentials, then it's fine to do it on that call.
2 participants
Towards #3646