Skip to content
This repository was archived by the owner on May 12, 2026. It is now read-only.

feat: update retries and implement Retryable - #750

Merged
TimurSadykov merged 39 commits into
mainfrom
stim-common-retry
Feb 7, 2022
Merged

feat: update retries and implement Retryable#750
TimurSadykov merged 39 commits into
mainfrom
stim-common-retry

Conversation

@TimurSadykov

@TimurSadykov TimurSadykov commented Sep 28, 2021

Copy link
Copy Markdown

This is PR fixes retries to token endpoint.
For more details on the feature: go/auth-correct-retry

Fixes #626

@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 28, 2021
@TimurSadykov
TimurSadykov marked this pull request as ready for review January 7, 2022 13:34
@TimurSadykov
TimurSadykov requested review from a team and arithmetic1728 January 7, 2022 13:34
Timur Sadykov and others added 6 commits January 9, 2022 18:47
Comment thread oauth2_http/java/com/google/auth/oauth2/GoogleAuthException.java Outdated
Comment thread oauth2_http/java/com/google/auth/oauth2/GoogleAuthException.java Outdated
Comment thread oauth2_http/java/com/google/auth/oauth2/GoogleAuthException.java Outdated
Comment thread credentials/java/com/google/auth/Retryable.java
Comment thread oauth2_http/java/com/google/auth/oauth2/GoogleCredentials.java

@lesv lesv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to spend a bit more time w/ your testing, but I'm generally favorable. If I didn't comment on Jeff's comment's, I probably agree with them. Even were I disagree, I'll defer to him if he insists.

ServiceAccountCredentials constructor has way too many parameters and should be made into a builder pattern - especially since your breaking any contract anyway.

I want to spend a bit more time thinking through your tests, but I believe you've implemented what I read in the design docs.

Comment thread credentials/java/com/google/auth/Retryable.java
Comment thread oauth2_http/java/com/google/auth/oauth2/GoogleAuthException.java Outdated
Comment thread oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java Outdated
Comment thread oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java Outdated
Comment thread oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java Outdated
Comment thread oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java Outdated
Comment thread oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java Outdated
Comment thread oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java Outdated
@TimurSadykov TimurSadykov self-assigned this Feb 4, 2022

@lesv lesv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sending this early as I'm going to get lunch.

I found some stuff that you will want to change. I'm suggesting change to the form of the builder pattern that I ok'd yesterday - apologies - I should n't type on my phone. I'm ok if you don't want to do it. I'm unattached to my suggestions on names.

I'll pick up where I left off in an hour or so.

@lesv lesv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore my earlier comments. LGTM

@TimurSadykov
TimurSadykov merged commit f9a9b8a into main Feb 7, 2022
@TimurSadykov
TimurSadykov deleted the stim-common-retry branch February 7, 2022 04:51

@ejona86 ejona86 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drive-by comment


package com.google.auth;

// an interface to identify retryable errors

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/** Retryable is an interface to identify retryable errors. */

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RequestMetadataCallback.onFailure() should provide a type-safe method to retrieve the root cause of the failure

7 participants