Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: update retries and implement Retryable #750

Merged
merged 39 commits into from
Feb 7, 2022
Merged

Conversation

TimurSadykov
Copy link
Contributor

@TimurSadykov TimurSadykov commented Sep 28, 2021

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 a review from a team as a code owner January 7, 2022 13:34
Copy link
Contributor

@lesv lesv left a comment

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.

@TimurSadykov TimurSadykov self-assigned this Feb 4, 2022
Copy link
Contributor

@lesv lesv left a comment

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.

Copy link
Contributor

@lesv lesv left a comment

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
Copy link
Contributor

@ejona86 ejona86 left a comment

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
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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
7 participants