andygrove commented on code in PR #6509:
URL: https://github.com/apache/datafusion-comet/pull/6509#discussion_r4157106089
##########
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:
Could we add an Iceberg version of this test? Most of the gain in the
description is on the Iceberg path. I tried one locally on 4.1 with a one-hour
expiry. It made no provider calls after the warm-up scan with this PR, and 20
calls with reuse disabled. That would catch a `FileIO` cache change that
quietly breaks the reuse.
##########
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:
Two spots further down still describe the old behavior. "When Comet asks"
says a location you drop stops being used once its credential fails. With
reuse, the location's bridge keeps serving its kept credential without asking
until five minutes before it expires, so a dropped location stays in use until
then. The reference implementation block still returns `0L`, while the
`spark/src/test` copy the guide points to now publishes the expiry, so vendors
who copy the guide turn reuse off. Could you update both? The design doc
heading "The IRSA web-identity provider is the exception that does cache" also
contradicts the new section above it now.
--
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]
