parthchandra commented on PR #6025:
URL:
https://github.com/apache/datafusion-comet/pull/6025#issuecomment-5805507891
> I don't think the Parquet take-over can ever engage in a real session.
`NativeConfig.extractObjectStoreOptions` forwards everything
`hadoopConf.iterator()` returns, and that includes `core-default.xml`, which
ships a default `fs.s3a.aws.credentials.provider` (Temporary, Simple,
Environment, IAMInstance). I checked this against `hadoop-client-api` 3.4.1,
and our own `datasources.md` relies on that default when it says the provider
key can be omitted for static keys. So `explicit_provider` in `create_store` is
always true, and we stand aside with an info line saying an explicit provider
is configured when the user configured nothing. Parquet users on IRSA today
already have to name `WebIdentityTokenCredentialsProvider`, because the default
list never reaches web identity, and that counts as explicit too. Could the
Parquet side engage when the configured provider is the web-identity provider,
since that's the same role and token file? Or could we scope the Parquet wiring
and those
doc claims out of this PR?
>
> Two of the tests rebuild the production wiring instead of calling it, so
they stay green if the fix is reverted. `iceberg_key_lookup_uses_s3_prefix`
copies the closure from `build_s3_credential_loader`, and nothing calls
`build_s3_credential_loader` itself, so changing it back to `return Ok(None)`
passes every test. The retry tests build their own `SdkConfig` in
`entry_with_http` with `max_attempts = 3`, which is the
`RetryConfig::standard()` default, so dropping the `retry_config` line from
`build_provider` passes as well. Could the Iceberg test call
`build_s3_credential_loader` with IRSA set, and could `build_provider` accept
an optional HTTP client so a retry test goes through it and checks the default
of five attempts?
You're right on both counts — thanks for digging into the config flow.
**Parquet take-over removed.** Confirmed:
`NativeConfig.extractObjectStoreOptions` iterates
`hadoopConf.iterator()`, which includes `core-default.xml`'s default
`fs.s3a.aws.credentials.provider`, so `explicit_provider` is always true and
the Parquet branch
could never engage (and logged a misleading "explicit provider configured").
I've dropped the
Parquet wiring and the doc claims and scoped this PR to the Iceberg path,
which is where the
reported failure is. If we want Parquet coverage later we can do a followup.
**Tests now exercise production wiring.**
- The Iceberg test (`iceberg_wiring_reads_s3_prefixed_keys`) calls
`build_s3_credential_loader`
directly with IRSA set: it asserts the loader engages, that
`s3.comet.credential.webIdentity.enabled=false`
turns it off, and that a bare unprefixed key does not. Reverting the
wiring to `Ok(None)` or using
the wrong prefix now fails.
- The retry test goes through `build_provider` (which takes an optional HTTP
client for the stub)
with the default 5 attempts and asserts 5 STS calls, so dropping the
`retry_config` line (SDK
default 3) now fails.
--
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]
