eric-maynard commented on code in PR #2301:
URL: https://github.com/apache/polaris/pull/2301#discussion_r2270531500
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -220,42 +218,29 @@ 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()));
+ factoryIdentifier = "iceberg-rest";
Review Comment:
Not a fan of these magic strings which basically are just another flavor of
`connectionType`. Can we use `connectionType` directly or, barring that, use a
constant? e.g. `HadoopFederatedCatalogFactory.CATALOG_TYPE = "hadoop"`
--
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
