snmvaughan opened a new pull request, #6509:
URL: https://github.com/apache/datafusion-comet/pull/6509
## Which issue does this PR close?
Closes #6508.
## Rationale for this change
The bridge forwarded every credential request to the JVM provider, and the
two native paths honored the provider's `expirationEpochMillis` differently:
- **Parquet (`object_store`).** The bridge dropped the expiry, and
object_store asks for a credential on every request. So a scan called the
provider once per HTTP request and used a credential however close it was to
expiring. object_store also signs a request once and replays that signature on
every retry, for up to 3 minutes by default, and it doesn't retry a 403. A
retry after backoff could therefore arrive after the session token expired.
- **Iceberg (opendal and reqsign).** iceberg-rust builds a new operator, and
with it a new reqsign signer, for every storage call. reqsign's cache therefore
lasted one operation, and a scan called the provider at least once per data and
delete file, even with 1-hour tokens.
This PR takes the expiry-aware approach in the issue's proposal, rather than
a safety margin alone.
## What changes are included in this PR?
- **Credential reuse bounded by the reported expiry**
(`cloud/s3/credential_bridge.rs`). Each bridge keeps its last credential with a
known expiry and reuses it until 5 minutes before that expiry, then asks the
provider again. Concurrent requests on the bridge wait for that one call. Five
minutes is the refresh-ahead of Comet's other credential caches (the native
Parquet credential chain and the IRSA web-identity provider), and it covers
object_store's retries. Both paths go through it, so the provider is asked
about once per credential, instead of once per request (Parquet) or per storage
call (Iceberg).
- **Credentials that aren't kept.** A credential with an unknown expiry
(`0`), or one that doesn't expire, is fetched for every request as before. So a
provider that needs to see every request, or whose credentials can be revoked
early, can report `0`. A credential returned with less than 5 minutes left is
used for its request and not kept.
- **Odd expiry values.** `Long.MAX_VALUE` now means no expiry. Before, it
failed every Iceberg request with `failed to load signing credential`. A value
before 2000, which is almost always seconds sent as milliseconds, is treated as
unknown with a one-time warning. Before, it read as 1970 and failed every
Iceberg request.
- **JVM side.**
- The `IcebergRESTVendedS3Provider` example reports the vended
credential's expiry instead of `0`.
- The Spark 3.x adapter's comment no longer calls the 5-minute default
safe.
- The `CometS3Credentials` Javadoc documents the expiry semantics and the
reuse.
- **Docs.**
- The design notes' "Why no Comet-side cache" section becomes "Credential
reuse, bounded by the reported expiry", with the rationale above.
- The path table, the user guide's caching and expiry text, and a `FileIO`
cache comment are corrected. They claimed opendal "schedules the next refresh"
from the expiry, and that object_store's retry layer calls `get_credential()`
again after a 403. Neither is what the pinned versions do.
- The docs note that the Spark 3.4/3.5 adapters (AWS SDK v1) can't report
an expiry.
This reverses the documented "no Comet-side cache" rule. The reuse window
isn't a tuning knob: it follows the expiry the provider reports.
**Interaction with #6478.** Both PRs change `credential_bridge.rs`. #6478
adds `CometS3CredentialBridge::for_location`, which builds a bridge without
this PR's `cache` field. So whichever of the two merges second needs `cache:
CredentialCache::default()` added there, and I'll rebase it.
## How are these changes tested?
- **New Rust unit tests** in `cloud::s3::credential_bridge`. They run
without a JVM and cover:
- reading an expiry: `0` and negatives, seconds sent as milliseconds,
`Long.MAX_VALUE`, and a real expiry;
- reusing a credential until 5 minutes before its expiry, then fetching
the next one;
- fetching every time for an unknown expiry, a credential that never
expires, and one within 5 minutes of expiry;
- not keeping a failed fetch;
- concurrent requests waiting for one fetch.
- **Revert checks.** Disabling reuse, keeping credentials the cache
shouldn't, and dropping the 5-minute margin each fail the matching tests.
- **`CometS3CredentialBridgeSuite`** (MinIO, run manually). A new last test
has the provider report an expiry an hour ahead. After a warm-up read, it
checks that two more native Parquet reads don't call the provider at all. This
sends a real expiry through the JNI bridge. The suite needs Docker and hasn't
been run on this revision.
These pass locally:
- `cargo fmt`
- `cargo clippy --all-targets --workspace -- -D warnings`
- `cargo test -p datafusion-comet --lib` (583 passed)
- Spotless and Prettier
- `./mvnw test-compile` with JDK 17 (main and test sources, all modules)
--
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]
