laskoviymishka commented on code in PR #1579:
URL: https://github.com/apache/iceberg-go/pull/1579#discussion_r3719815501
##########
catalog/rest/vended_creds.go:
##########
@@ -136,7 +136,11 @@ func (v *vendedCredentialRefresher) loadFS(ctx
context.Context) (iceio.IO, error
default:
freshCreds, err := v.fetchCreds(ctx, v.identifier)
if err != nil {
- return v.cachedIO, nil
+ if !v.expired() {
Review Comment:
the `!v.expired()` guard here is dead code, we can't reach it in a
not-expired state.
the fast path at the top (`if v.cachedIO != nil && !v.expired()`) already
returned for everything that isn't expired, and we only reach `default:` when
`cachedIO != nil` and `fetchCreds != nil`. the semaphore's held the whole way
through, so `expiresAt` can't move and the clock only advances. by the time
we're in here `v.expired()` is always true, so `return v.cachedIO, nil` never
fires.
so the real question is the post-expiry policy. if it's "once we're past
expiry any refresh failure is terminal," I'd drop the guard and propagate
unconditionally. that's also closest to Java, which surfaces all refresh errors
through `defaultErrorHandler()` with no degradation. if instead we want to keep
serving cached creds through a refresh blip while they're still valid, that's a
proactive-refresh window and it has to live in `expiresAt` (a pre-expiry
threshold, or a `shouldProactivelyRefresh()` predicate), not a guard inside
`default:` that can only run after we're already expired.
which policy are you after here? wdyt?
##########
catalog/rest/vended_creds_test.go:
##########
@@ -153,7 +154,7 @@ func TestVendedCredsConcurrentAccess(t *testing.T) {
assert.NotNil(t, r.cachedIO, "cachedIO should be set after initial
load")
}
-func TestVendedCredsGracefulDegradation(t *testing.T) {
+func TestVendedCredsReturnsRefreshFailureForExpiredCredentials(t *testing.T) {
Review Comment:
repurposing this test to assert the error path means we've dropped the only
coverage of the degradation contract: refresh fails, cached IO comes back, no
error.
whichever behavior we settle on, I'd want it covered. a case with
`expiresAt` in the future plus a failing `fetchCreds`, asserting cached IO
returns with no error, would lock the contract and, handily, immediately show
whether that `!v.expired()` branch is actually reachable.
##########
catalog/rest/vended_creds_test.go:
##########
@@ -185,6 +205,7 @@ func TestVendedCredsErrorWhenInitialLoadFails(t *testing.T)
{
got, err := r.loadFS(context.Background())
require.Error(t, err)
+ assert.NotContains(t, err.Error(), "refreshed credentials")
Review Comment:
minor, not blocking: this matches on a substring of the message, so it'll
silently pass if that wrapper string ever changes. a sentinel error plus
`errors.Is` would be sturdier if you feel like it.
##########
catalog/rest/vended_creds.go:
##########
@@ -145,11 +149,14 @@ func (v *vendedCredentialRefresher) loadFS(ctx
context.Context) (iceio.IO, error
newIO, err := iceio.LoadFS(ctx, config, v.location)
if err != nil {
- if v.cachedIO != nil {
+ if v.cachedIO == nil {
+ return nil, err
+ }
+ if !v.expired() {
Review Comment:
same dead guard as line 139. after the `cachedIO == nil` early return,
`cachedIO != nil` is guaranteed, and in `default:` we're always expired, so
this `return v.cachedIO, nil` can't fire either. whichever way the policy above
lands, this branch should move the same way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
