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: Async refresh dry run in parallel with sync refresh #2849

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

bhshkh
Copy link
Contributor

@bhshkh bhshkh commented Oct 31, 2024

Internal bug : b/372244283

This is being used in Bigtable client
googleapis/google-cloud-go#11066

@bhshkh bhshkh changed the title Feat/async refresh dry run feat: Async refresh dry run in parallel with sync refresh Oct 31, 2024
@bhshkh bhshkh marked this pull request as ready for review November 4, 2024 19:33
@bhshkh bhshkh requested a review from a team as a code owner November 4, 2024 19:33
option/internaloption/internaloption.go Outdated Show resolved Hide resolved
option/internaloption/internaloption.go Outdated Show resolved Hide resolved
transport/grpc/dial.go Outdated Show resolved Hide resolved
option/internaloption/internaloption.go Outdated Show resolved Hide resolved
// module should asynchronously refresh auth token in parallel to sync refresh
//
// errHandler function will be called when there is an error while refreshing
// the token asynchronously
Copy link
Member

Choose a reason for hiding this comment

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

What should the errHandler function do? Log the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific requirement on that. User can do whatever they need to

internal/settings.go Show resolved Hide resolved
@bhshkh bhshkh requested a review from quartzmo November 26, 2024 19:04
@codyoss
Copy link
Member

codyoss commented Nov 26, 2024

Can this change be made upstream instead with an embedded private option: https://pkg.go.dev/google.golang.org/api/option/internaloption#EmbeddableAdapter

@bhshkh
Copy link
Contributor Author

bhshkh commented Nov 27, 2024

Can this change be made upstream instead with an embedded private option: https://pkg.go.dev/google.golang.org/api/option/internaloption#EmbeddableAdapter

No. the token refresh code is deep into this library and not Bigtable.

//
// This is an EXPERIMENTAL option and will be removed in the future.
// TODO(b/372244283): Remove after b/358175516 has been fixed
func EnableAsyncRefreshDryRun(errHandler func()) option.ClientOption {
Copy link

Choose a reason for hiding this comment

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

Is there any annotation to mark this as internal api?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No.

@@ -298,8 +332,14 @@ func dial(ctx context.Context, insecure bool, o *internal.DialSettings) (*grpc.C
if err != nil {
return nil, err
}

ts := creds.TokenSource
Copy link
Member

Choose a reason for hiding this comment

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

This code will not be hit if the new auth lib is enabled. That that intentional here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants