eric-maynard commented on code in PR #2301:
URL: https://github.com/apache/polaris/pull/2301#discussion_r2271081822
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -220,42 +218,27 @@ protected void initializeCatalog() {
ConnectionType connectionType =
ConnectionType.fromCode(connectionConfigInfoDpo.getConnectionTypeCode());
+ // Use the unified factory pattern for all external catalog types
+ String factoryIdentifier;
switch (connectionType) {
case ICEBERG_REST:
- SessionCatalog.SessionContext context =
SessionCatalog.SessionContext.createEmpty();
- federatedCatalog =
- new RESTCatalog(
- context,
- (config) ->
- HTTPClient.builder(config)
-
.uri(config.get(org.apache.iceberg.CatalogProperties.URI))
- .build());
- federatedCatalog.initialize(
- ((IcebergRestConnectionConfigInfoDpo)
connectionConfigInfoDpo).getRemoteCatalogName(),
-
connectionConfigInfoDpo.asIcebergCatalogProperties(getUserSecretsManager()));
- break;
case HADOOP:
- // Currently, Polaris supports Hadoop federation only via IMPLICIT
authentication.
- // Hence, prior to initializing the configuration, ensure that the
catalog uses
- // IMPLICIT authentication.
- AuthenticationParametersDpo authenticationParametersDpo =
- connectionConfigInfoDpo.getAuthenticationParameters();
- if (authenticationParametersDpo.getAuthenticationTypeCode()
- != AuthenticationType.IMPLICIT.getCode()) {
- throw new IllegalStateException(
- "Hadoop federation only supports IMPLICIT authentication.");
- }
- Configuration conf = new Configuration();
- String warehouse =
- ((HadoopConnectionConfigInfoDpo)
connectionConfigInfoDpo).getWarehouse();
- federatedCatalog = new HadoopCatalog(conf, warehouse);
- federatedCatalog.initialize(
- warehouse,
-
connectionConfigInfoDpo.asIcebergCatalogProperties(getUserSecretsManager()));
+ factoryIdentifier = connectionType.getFactoryIdentifier();
break;
default:
- throw new UnsupportedOperationException(
- "Connection type not supported: " + connectionType);
+ throw new UnsupportedOperationException("Unsupported connection
type: " + connectionType);
Review Comment:
Sorry to keep asking for changes here, but it looks like the `switch` is
basically not needed anymore. Can we try to remove the `switch`, add error
handling to `getFactoryIdentifier` and use something like:
```java
Instance<ExternalCatalogFactory> externalCatalogFactory =
externalCatalogFactoryRegistry.select(connectionType.
getFactoryIdentifier())
```
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
