FANNG1 commented on PR #11294:
URL: https://github.com/apache/gravitino/pull/11294#issuecomment-4620829507
Thanks, the new `SupportsStorageCredentials#credentials()` path looks like
the right direction and the added tests cover the structured
`storage-credentials` case.
One remaining edge case: `buildStorageCreds()` currently normalizes every
upstream `StorageCredential.prefix()` through `storageLocationPrefix()`, which
appends `/` when the prefix does not already end with one. That is fine for
table/location-style prefixes such as `s3://bucket/path`, but it changes
Iceberg root scheme prefixes like `s3` into `s3/`.
Iceberg S3 vended credentials use `prefix("s3")` in its own tests, and
`S3FileIO.clientForStoragePath()` matches credentials by checking
`storagePath.startsWith(storagePrefix)`. After rewriting `s3` to `s3/`, a
normal storage path like `s3://bucket/...` no longer matches that credential,
so the client can fall back to the default S3 client instead of the vended
credential.
Could we avoid normalizing upstream `StorageCredential.prefix()` values when
forwarding them? The trailing-slash normalization is useful for locally
generated table-location prefixes, but forwarded upstream credential prefixes
should probably be preserved as-is, or at least root scheme prefixes like `s3`
/ `gs` should be left unchanged.
Minor nit while touching this area: `AwsIrsaCredential` now says `should
great than 0`; should be `should be greater than 0`.
--
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]
