andygrove opened a new issue, #6231:
URL: https://github.com/apache/datafusion-comet/issues/6231
These are left over from the review of #6025, which adds a process-wide
web-identity credential provider for the native Iceberg S3 path in
`native/core/src/cloud/s3/web_identity.rs`. We agreed they could land
separately so that PR isn't held up on them.
The one with user impact is how readers behave during a refresh. Once the
cached credential is inside `minTtlSeconds` of expiry (300s by default),
`SharedEntry::credentials` sends every caller to `refresh_lock`. The holder
then runs `AssumeRoleWithWebIdentity` through the SDK's retry loop, which under
a throttle is five attempts with up to about 15 seconds of backoff. Every other
caller waits behind it, even though `still_signable()` would give them a
credential with minutes left. That covers every native Iceberg file operation
in the executor, because `iceberg-storage-opendal` builds a new operator for
each storage call (`exists`, `metadata`, `read`, `reader`, `write` and the
rest). Each new operator has a fresh reqsign `Signer` with an empty cache, so
each call comes back to our provider.
The commit that fixed the signing dead zone (df2197b0f on the current
branch) also removed the refresh jitter and the `refreshJitterSeconds` knob,
and the PR thread doesn't say why. Before that, each entry added up to 60
seconds of random jitter to `minTtlSeconds`, so executors that assumed the role
in the same startup burst didn't all refresh in the same second an hour later.
Now they do. Single-flight keeps that to one STS call per executor, but with
the stall above, a throttled refresh would stall every executor at once. The
jitter only moves our own refresh earlier, and the provider now reports the
real expiry, so bringing it back can't reopen the dead zone. The comment in
`build_s3_credential_loader` in `iceberg_common.rs` still describes a "shared
jittered cache".
Two smaller things in the tests. `concurrent_failed_refresh_is_coalesced`
says every waiter must see the real cause, but it only asserts on `web-identity
assume-role failed`, which `credentials()` prepends itself and the cooldown
replay keeps. It would still pass if the replay dropped the cause. And two
comments in `iceberg_wiring_reads_s3_prefixed_keys` say a bare
`comet.credential.webIdentity.enabled` key never reaches the catalog property
bag. It does reach it, because Comet forwards the unfiltered FileIO properties,
and it has no effect only because the lookup adds the `s3.` prefix. The doc
comment on the key constants and the user guide already say that correctly.
The work:
- While a refresh is in flight, callers that don't hold `refresh_lock` get
the cached credential if it is still signable, instead of waiting on STS.
- Bring back a per-entry refresh jitter, or explain in the code why it isn't
needed, and fix the "jittered cache" comment either way.
- Make `concurrent_failed_refresh_is_coalesced` assert on the stub
provider's own error text ("the credential provider was not enabled").
- Fix the two bare-key comments in `iceberg_wiring_reads_s3_prefixed_keys`.
Done when a test with a slow, throttled refresh shows the other callers
getting the cached credential without waiting, and the other three items are in.
--
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]
