Datalake catalog auth token profile events - #2010
Conversation
Track how often REST-family catalogs reuse cached OAuth/GCP tokens versus fetching new ones or retrying after HTTP 401/403, and add an integration test for the new metrics. Co-authored-by: Cursor <cursoragent@cursor.com>
Use a mock OAuth server in the lakekeeper compose stack and valid client credentials so token refresh and cache-hit profile events are observed on SHOW TABLES queries. Co-authored-by: Cursor <cursoragent@cursor.com>
Treat expired OAuth tokens as refresh candidates instead of cache hits, and count BigLake GCP token refresh once per logical fetch when ADC fallback to metadata is used. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2a7bab07f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| else | ||
| { | ||
| ProfileEvents::increment(ProfileEvents::DataLakeRestCatalogAuthTokenCacheHits); |
There was a problem hiding this comment.
When a cached token is rejected with HTTP 401/403, the retry paths in createReadBuffer and sendRequest immediately fetch a new token, but this increment has already recorded DataLakeRestCatalogAuthTokenCacheHits. In that stale/revoked-token scenario the same query reports both a cache hit and a refresh even though the event is documented as reusing a cached token without fetching a new one; move this accounting to a point where the catalog request has succeeded, or suppress it when the unauthorized retry runs.
Useful? React with 👍 / 👎.
Defer DataLakeRestCatalogAuthTokenCacheHits until createReadBuffer or sendRequest completes without a 401/403 retry, so stale cached tokens do not report both a cache hit and a refresh in the same query. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| { | ||
| access_token = retrieveAccessToken(); | ||
| } | ||
| else if (used_cached_oauth_token) |
There was a problem hiding this comment.
hm.. this is a bit confusing, tho I understand you are checking if the pointer is valid. Can't we pass a reference instead?
There was a problem hiding this comment.
Method is overridden in S3TablesCatalog, and there used_cached_oauth_token is not used.
| return create_buffer(false); | ||
| bool used_cached_oauth_token = false; | ||
| auto buf = create_buffer(false, used_cached_oauth_token); | ||
| if (used_cached_oauth_token) |
There was a problem hiding this comment.
Why not increment DataLakeRestCatalogAuthTokenCacheHits where it actually hits the cache instead of passing this used_cache argument around?
There was a problem hiding this comment.
It is not an event about caching, it is event about token requests.
getAuthHeaders can return cached token, but we don't know if it correct or not, it can be revoked. So counter increased only when token is successfully used in BuilderRWBufferFromHTTP. In other case we can get an error and have to request new token.
2 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Datalake catalog auth token profile events
Documentation entry for user-facing changes
New profile events for requests to catalog token, REST only.
CI/CD Options
Exclude tests:
Regression jobs to run: