laxmanmamididd commented on PR #7363: URL: https://github.com/apache/datafusion-comet/pull/7363#issuecomment-6614671326
Thanks for running these. Changes are pushed. **Splitting.** The lifecycle change is now #7464, which closes #7390: the dispatcher reference counts, `release` and its JNI binding, `ProviderHandle`, the FileIO cache eviction and the design doc text. This branch is rebased on it. Until it merges, this PR's diff includes its two commits. The provider changes are the last three. **1. Eviction on equal properties** (in #7464). `supersedes` now requires different properties, so a commit that keeps them evicts nothing. For two sessions with different tokens, I went with your ordering: when both entries carry `s3.session-token-expires-at-ms`, only the later expiry supersedes. When either lacks it, a properties change still evicts. The unreachable `self != other` is gone. New Rust tests cover a commit with unchanged properties and the expiry ordering, and the design doc and user guide describe the rule. **2. Write path.** I dropped the write change. `requireSupportedS3FileIOProperties` rejects `client.refresh-credentials-endpoint` first, so such a write falls back to Spark either way. The provider section of `s3-credential-providers.md` now says reads only, and that the writer falls back. Write support can follow with a `CometIcebergWriteDetectionSuite` case. **3. Failed initialization retried per task.** Selection now checks on the driver, once per JVM: - the provider class loads from Comet's loader; - `AwsClientProperties` loads from the context loader; - the `AwsCredentialsProvider` that `AwsClientProperties` links against is the same class Comet's loader sees. A relative endpoint with no `uri` isn't selected either. When the check fails, Comet logs once at `INFO` and keeps the vended credentials. I left the native read fallback uncached. `cached_file_io_does_not_cache_a_degraded_build` pins that behavior from #6478, and caching it would keep a FileIO that skips the configured provider until it's evicted. Happy to change it if you'd rather cache it. **4. Wiring tests.** - `CometIcebergNativeScanSuite` calls the public overload, so a wrong `VendedCredentialProviderClass` fails: `Some(...)` on 4.x, `None` on 3.x. - `CometIcebergNativeSuite` plans and runs a native scan against a REST catalog that returns a refresh endpoint, and checks the provider key in `nativeIcebergScanMetadata.catalogProperties`. - In #7464, `CometS3CredentialBridgeSuite` reads one table from two sessions whose catalogs give it different FileIO properties, and checks that `registeredCount()` doesn't grow. With eviction disabled it fails with 2 registered providers instead of 1. **5. Docs.** `iceberg.md` now says the native reader on Spark 4.x refreshes vended credentials through this provider when the catalog returns a refresh endpoint. -- 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]
