dennishuo commented on code in PR #2301:
URL: https://github.com/apache/polaris/pull/2301#discussion_r2271223467
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -220,42 +218,18 @@ protected void initializeCatalog() {
ConnectionType connectionType =
ConnectionType.fromCode(connectionConfigInfoDpo.getConnectionTypeCode());
- 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()));
- break;
- default:
- throw new UnsupportedOperationException(
- "Connection type not supported: " + connectionType);
+ // Use the unified factory pattern for all external catalog types
+ Instance<ExternalCatalogFactory> externalCatalogFactory =
+ externalCatalogFactories.select(
+ Identifier.Literal.of(connectionType.getFactoryIdentifier()));
+ if (!externalCatalogFactory.isUnsatisfied()) {
Review Comment:
Generally I like to avoid double-negatives where possible, so could be worth
considering swapping the `if/else` here to get rid of the double-negative.
##########
runtime/service/build.gradle.kts:
##########
@@ -30,6 +30,9 @@ dependencies {
implementation(project(":polaris-api-management-service"))
implementation(project(":polaris-api-iceberg-service"))
implementation(project(":polaris-api-catalog-service"))
+ if ((project.findProperty("NonRESTCatalogs") as String?)?.contains("HADOOP")
== true) {
Review Comment:
+1 to adding something to the README/getting-started or similar to better
make it an easy copy/paste command to decide whether to compile with or without
the extended dependencies.
My understanding is indeed that this check was directly to address the
concern others had about having Hadoop (or Hive in the future) compile-time
dependencies be always present for all Polaris builds.
Personally I don't feel too strongly either way, so I'm okay with or without
the additional compilation property.
--
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]
