add refresh credentials property to loadTableResult #2341
Conversation
jasonf20
force-pushed
the
loadTableResponse-refresh-credential-properties
branch
from
bb55a80 to
eb3e360
Compare
…ndpoint is included
| LoadTableResponse.builder().withTableMetadata(originalResponse.tableMetadata()); | ||
| loadResponseBuilder.addAllConfig(originalResponse.config()); | ||
| loadResponseBuilder.addAllCredentials(originalResponse.credentials()); | ||
| loadResponseBuilder.addConfig( |
There was a problem hiding this comment.
what about azure ? do we want to do it in follow-up ? also should we only inject this when creds are for aws ?
There was a problem hiding this comment.
FWIW, I think enabling the Azure refresh properties currently causes Azure credential vending to immediately fail due to apache/iceberg#13733, so could maybe wait for that fix to release?
There was a problem hiding this comment.
also should we only inject this when creds are for aws?
Could maybe lift the AWS refresh check below to loadTable itself, to avoid e.g. reconstructing the LoadTableResponse if not needed?
There was a problem hiding this comment.
I think it's better to provide the URL even if it's not AWS that way the user can override AwsClientProperties.REFRESH_CREDENTIALS_ENABLED if he is working with a system that he knows supports it despite the server not being updated to reflect this yet. For example, an updated Azure iceberg SDK like mentioned above.
| private LoadTableResponse injectRefreshVendedCredentialProperties( | ||
| LoadTableResponse originalResponse, String credentialsEndpoint) { | ||
| LoadTableResponse.Builder loadResponseBuilder = | ||
| LoadTableResponse.builder().withTableMetadata(originalResponse.tableMetadata()); |
There was a problem hiding this comment.
do we need to update metadata-location too ?
There was a problem hiding this comment.
Update it to what value?
Do you mean is it redundant with the copyConfigs below? Seems like the metadata location is stored in a dedicated field and is not part of the config.
Rebased version of #1164