snmvaughan commented on code in PR #6509:
URL: https://github.com/apache/datafusion-comet/pull/6509#discussion_r4158554449
##########
docs/source/user-guide/latest/s3-credential-providers.md:
##########
@@ -239,20 +239,22 @@ public CometS3Credentials
getCredentialsForPath(CometS3CredentialContext ctx) th
Spark delegation token propagation is supported on YARN and Kubernetes only.
Standalone deployments need a different refresh path, typically a vendor-side
service callback authenticated by long-lived state in `catalogProperties` or
Hadoop conf.
-`expirationEpochMillis` only matters on the Iceberg/`opendal` path. There the
bridge implements `reqsign_core::ProvideCredential`, which carries an
`expires_in` field that `opendal` uses to schedule the next refresh. Publish a
real expiry when you have one. `0` means "unknown"; the bridge then substitutes
a 5-minute expiry to bound staleness.
+Publish a real `expirationEpochMillis` when you have one. On both paths Comet
reuses a credential until five minutes before that expiry and then asks you
again, and requests that arrive together wait for that one call. `0` means
unknown: Comet does not keep the credential and asks you for every request, and
on the Iceberg path it assumes the credential lasts five minutes.
`Long.MAX_VALUE` means the credential does not expire, and Comet does not keep
it either. A value before 2000, almost always seconds sent as milliseconds, is
treated as unknown, with a warning. If a credential can be revoked before the
expiry you report, report an earlier one, or `0`.
Review Comment:
Thanks, all three are fixed in 9ecdbcd38. "When Comet asks" now says a
dropped location stays in use until Comet next asks for its credential, five
minutes before the expiry reported for it, or at the next request for `0`. Both
example providers in the guide, the fallback to the default chain as well as
the reference implementation, now report the session credential's expiry like
the copy under `spark/src/test`, and the paragraph after the reference
implementation says that returning `0` turns reuse off. The IRSA heading is now
"The IRSA web-identity provider keeps its own cache".
##########
spark/src/test/scala/org/apache/comet/cloud/s3/CometS3CredentialBridgeSuite.scala:
##########
@@ -315,4 +315,31 @@ class CometS3CredentialBridgeSuite
"/warehouse/finance"),
s"Unexpected credential paths:
${MinioLocationScopedCredentialProvider.credentialPaths()}")
}
+
+ // Declared last: the bridge keeps the credential this test reports an
expiry for, so a later
+ // Parquet test on this bucket would not see the provider asked.
+ test("Parquet reads reuse a credential until shortly before its expiry") {
Review Comment:
Good idea, added in 9ecdbcd38. `Iceberg reads reuse a credential until
shortly before its expiry` warms up with a one-hour expiry, then checks that
two more native Iceberg scans make no provider calls, so a `FileIO` cache
change that rebuilt the bridge would fail it. I can't run the suite where I am,
so a run against S3Proxy would be welcome if you have it set up.
--
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]
