lasdf1234 commented on PR #11294:
URL: https://github.com/apache/gravitino/pull/11294#issuecomment-4622087429
> should be greater than 0
> 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`.
Regarding the first issue, I carefully reviewed the source code of iceberg
and conducted local tests. I found that the "/ " at the end of "location" was
not necessary, so I removed this logic and thus this problem no longer exists.
Regarding the second question, I have revised the prompt information, and
the prompts for other token classes also align with your requirements.
--
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]
