adutra commented on code in PR #3480:
URL: https://github.com/apache/polaris/pull/3480#discussion_r2709006757
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRESTExternalCatalogFactory.java:
##########
@@ -54,18 +65,45 @@ public Catalog createCatalog(
.uri(config.get(org.apache.iceberg.CatalogProperties.URI))
.build());
- federatedCatalog.initialize(
- icebergConfig.getRemoteCatalogName(),
- connectionConfig.asIcebergCatalogProperties(polarisCredentialManager));
+ // Merge properties with precedence:
+ // 1. Start with ExternalCatalog.properties (pass-through for proxy,
timeouts, etc.)
+ // 2. Overlay with connectionConfig properties (URI, auth, etc.) which
take precedence
+ Map<String, String> mergedProperties =
+ mergeCatalogProperties(connectionConfig, polarisCredentialManager,
catalogProperties);
+
+ federatedCatalog.initialize(icebergConfig.getRemoteCatalogName(),
mergedProperties);
return federatedCatalog;
}
@Override
public GenericTableCatalog createGenericCatalog(
ConnectionConfigInfoDpo connectionConfig, PolarisCredentialManager
polarisCredentialManager) {
+ return createGenericCatalog(connectionConfig, polarisCredentialManager,
null);
+ }
+
+ @Override
+ public GenericTableCatalog createGenericCatalog(
+ ConnectionConfigInfoDpo connectionConfig,
+ PolarisCredentialManager polarisCredentialManager,
+ Map<String, String> catalogProperties) {
// TODO implement
throw new UnsupportedOperationException(
"Generic table federation to this catalog is not supported.");
}
+
+ @VisibleForTesting
+ static Map<String, String> mergeCatalogProperties(
Review Comment:
There is already a method `org.apache.iceberg.rest.RESTUtil#merge`, FYI. You
could replace this method with:
```java
Map<String, String> mergedProperties =
RESTUtil.merge(
catalogProperties == null ? Map.of() : catalogProperties,
connectionConfig.asIcebergCatalogProperties(polarisCredentialManager));
```
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/ExternalCatalogFactory.java:
##########
@@ -30,27 +33,96 @@
*/
public interface ExternalCatalogFactory {
+ Logger LOG = LoggerFactory.getLogger(ExternalCatalogFactory.class);
Review Comment:
It's a bit odd to expose a logger in an interface. Mind inlining the field?
##########
extensions/federation/hive/src/main/java/org/apache/polaris/extensions/federation/hive/HiveFederatedCatalogFactory.java:
##########
@@ -69,14 +79,27 @@ public Catalog createCatalog(
// Polaris could support federating to multiple LDAP based Hive
metastores. Multiple
// Kerberos instances are not suitable because Kerberos ties a single
identity to the server.
HiveCatalog hiveCatalog = new HiveCatalog();
- hiveCatalog.initialize(
- warehouse,
connectionConfigInfoDpo.asIcebergCatalogProperties(polarisCredentialManager));
+ Map<String, String> mergedProperties = new HashMap<>();
Review Comment:
Same here, you could replace with `RESTUtil.merge()` (even if `RESTUtil` is
primarily intended for the REST catalog, not Hive).
##########
extensions/federation/hadoop/src/main/java/org/apache/polaris/extensions/federation/hadoop/HadoopFederatedCatalogFactory.java:
##########
@@ -53,17 +63,34 @@ public Catalog createCatalog(
!= AuthenticationType.IMPLICIT.getCode()) {
throw new IllegalStateException("Hadoop federation only supports
IMPLICIT authentication.");
}
- Configuration conf = new Configuration();
String warehouse = ((HadoopConnectionConfigInfoDpo)
connectionConfigInfoDpo).getWarehouse();
- HadoopCatalog hadoopCatalog = new HadoopCatalog(conf, warehouse);
- hadoopCatalog.initialize(
- warehouse,
connectionConfigInfoDpo.asIcebergCatalogProperties(polarisCredentialManager));
+ Map<String, String> mergedProperties = new HashMap<>();
Review Comment:
Ditto.
--
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]
