gabeiglio commented on code in PR #3751:
URL: https://github.com/apache/iceberg-python/pull/3751#discussion_r3958819578
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -827,37 +812,50 @@ def add_headers(self, request: PreparedRequest, **kwargs:
Any) -> None: # pylin
def _response_to_table(self, identifier_tuple: tuple[str, ...],
table_response: TableResponse) -> Table:
# Per Iceberg spec: storage-credentials take precedence over config
- credential_config = self._resolve_storage_credentials(
- table_response.storage_credentials,
table_response.metadata_location
+ credential_config =
resolve_storage_credentials(table_response.storage_credentials,
table_response.metadata_location)
+ io = self._load_file_io(
+ {**table_response.metadata.properties, **table_response.config,
**credential_config},
+ table_response.metadata_location,
)
+ self._attach_credentials_provider(io, identifier_tuple,
table_response.storage_credentials)
return Table(
identifier=identifier_tuple,
metadata_location=table_response.metadata_location, # type: ignore
metadata=table_response.metadata,
- io=self._load_file_io(
- {**table_response.metadata.properties,
**table_response.config, **credential_config},
- table_response.metadata_location,
- ),
+ io=io,
catalog=self,
config=table_response.config,
)
def _response_to_staged_table(self, identifier_tuple: tuple[str, ...],
table_response: TableResponse) -> StagedTable:
# Per Iceberg spec: storage-credentials take precedence over config
- credential_config = self._resolve_storage_credentials(
- table_response.storage_credentials,
table_response.metadata_location
+ credential_config =
resolve_storage_credentials(table_response.storage_credentials,
table_response.metadata_location)
+ io = self._load_file_io(
+ {**table_response.metadata.properties, **table_response.config,
**credential_config},
+ table_response.metadata_location,
)
+ self._attach_credentials_provider(io, identifier_tuple,
table_response.storage_credentials)
return StagedTable(
identifier=identifier_tuple,
metadata_location=table_response.metadata_location, # type: ignore
metadata=table_response.metadata,
- io=self._load_file_io(
- {**table_response.metadata.properties,
**table_response.config, **credential_config},
- table_response.metadata_location,
- ),
+ io=io,
catalog=self,
)
+ def _attach_credentials_provider(
+ self, io: FileIO, identifier: str | Identifier, storage_credentials:
list[StorageCredential]
+ ) -> None:
+ """Attach a CredentialsProvider to io if credential refresh is enabled
and credentials were vended.
+
+ The refresh callback returns the full LoadCredentialsResponse so the
provider can re-run
+ longest-prefix matching against the freshly vended credentials.
+ """
+ if storage_credentials and property_as_bool(self.properties,
REFRESH_CREDENTIALS_ENABLED, False):
+ io.set_credentials_provider(
Review Comment:
Dropping and recreating the lock makes sense. My concern is that the refresh
lambda still captures the catalog instance, and the catalog contains additional
state like sessions, auth managers, and possibly other locks that may not be
pickleable.
So we’d still need to make the refresh call independent of the catalog or
reconstruct it after unpickling/
--
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]
