Repository navigation
Conversation
… API calls The GetPublicClientConfig gRPC call returns static server configuration (client ID, redirect URI, scopes, audience, auth metadata key) that does not change at runtime. It was being called up to 4 times during client initialization. This wraps the AuthMetadataServiceClient returned by InitializeAuthMetadataClient with a sync.Once-based caching decorator so the RPC is made at most once per client lifetime. Closes flyteorg#6868 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7054 +/- ##
=======================================
Coverage 56.95% 56.95%
=======================================
Files 931 932 +1
Lines 58188 58195 +7
=======================================
+ Hits 33141 33147 +6
- Misses 22005 22006 +1
Partials 3042 3042
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces redundant Flyte Admin RPC traffic during client initialization by caching the AuthMetadataServiceClient.GetPublicClientConfig response, since it represents static server-side configuration.
Changes:
- Added a
cachedMetadataClientdecorator that memoizesGetPublicClientConfigresults. - Wrapped the auth metadata client created by
InitializeAuthMetadataClientwith the caching decorator.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
flyteidl/clients/go/admin/client.go |
Wraps the created auth metadata client with the caching decorator so downstream callers automatically benefit. |
flyteidl/clients/go/admin/cached_metadata_client.go |
Implements the caching decorator using sync.Once to avoid redundant GetPublicClientConfig RPCs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A failed GetPublicClientConfig call (transient RPC error, cancelled context) was cached forever by sync.Once. Guard the cache with a mutex and store only successful responses so later calls retry. Add tests for concurrent callers and retry after errors. Signed-off-by: Matt Van Horn <mvanhorn@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tracking issue
Closes #6868
Why are the changes needed?
GetPublicClientConfigis called up to 4 times during client initialization (token_source_provider.go:64,client.go:100,auth_interceptor.go:142,oauth/config.go:22). The response contains static server configuration (client ID, redirect URI, scopes, audience, authorization metadata key) that does not change at runtime.As noted in the issue, this shows up as unnecessary API call volume on monitoring dashboards. The success criteria from the issue is reducing these calls to 0 after the first fetch.
What changes were proposed in this pull request?
Added a
cachedMetadataClientdecorator inflyteidl/clients/go/admin/cached_metadata_client.gothat wrapsAuthMetadataServiceClientand caches theGetPublicClientConfigresponse usingsync.Once. The first call makes the RPC; subsequent calls return the cached result.The decorator is applied in
InitializeAuthMetadataClient(client.go:124), which is the single point where the auth metadata client is created. All 4 downstream call sites automatically benefit from caching without any changes.GetOAuth2Metadatais not cached in this PR since it's called less frequently and serves a different purpose, but the same pattern could be applied if needed.How was this patch tested?
go fmt ./...andgo vet ./clients/go/admin/...pass cleansync.Oncepattern is well-established for this use case. The cached value is immutable after first write, so there are no concurrency concerns.This contribution was developed with AI assistance (Claude Code + Codex).
Labels