phet commented on code in PR #3643:
URL: https://github.com/apache/gobblin/pull/3643#discussion_r1103489061
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergCatalogFactory.java:
##########
@@ -18,19 +18,20 @@
package org.apache.gobblin.data.management.copy.iceberg;
import org.apache.hadoop.conf.Configuration;
-import org.apache.iceberg.hive.HiveCatalog;
+import org.apache.iceberg.CatalogUtil;
+import org.apache.iceberg.catalog.Catalog;
import com.google.common.collect.Maps;
+import org.apache.gobblin.util.reflection.GobblinConstructorUtils;
+
/**
* Provides an {@link IcebergCatalog}.
*/
public class IcebergCatalogFactory {
- public static IcebergCatalog create(Configuration configuration) {
- HiveCatalog hcat = new HiveCatalog();
- hcat.setConf(configuration);
- hcat.initialize("hive", Maps.newHashMap());
- return new IcebergHiveCatalog(hcat);
+ public static IcebergCatalog create(IcebergCatalog.CatalogSpecifier
specifier, Configuration configuration) throws ReflectiveOperationException {
Review Comment:
seems atypical to let `ReflectiveOperationException` escape. more commonly
handled within by the direct invoker of methods that throw
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergCatalog.java:
##########
@@ -17,10 +17,23 @@
package org.apache.gobblin.data.management.copy.iceberg;
+import org.apache.iceberg.catalog.Catalog;
+
/**
* Any catalog from which to access {@link IcebergTable}s.
*/
public interface IcebergCatalog {
IcebergTable openTable(String dbName, String tableName);
+
+ /**
+ * Adding a sub interface to help us provide an association between {@link
Catalog} and {@link IcebergCatalog}.
+ * This helps us resolve to the Catalog type and its concrete implementation
class
+ */
+ interface CatalogSpecifier {
+ Class<? extends Catalog> getCatalogClass();
+ Class<? extends IcebergCatalog> getIcebergCatalogClass();
+ String getCatalogType();
Review Comment:
type or description... or name?
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDatasetFinder.java:
##########
@@ -47,6 +49,8 @@ public class IcebergDatasetFinder implements
IterableDatasetFinder<IcebergDatase
public static final String ICEBERG_HIVE_CATALOG_METASTORE_URI_KEY =
ICEBERG_DATASET_PREFIX + ".hive.metastore.uri";
public static final String ICEBERG_DB_NAME = ICEBERG_DATASET_PREFIX +
".database.name";
public static final String ICEBERG_TABLE_NAME = ICEBERG_DATASET_PREFIX +
".table.name";
+ public static final String ICEBERG_CATALOG_SPECIFIER_CLASS_KEY =
DatasetConstants.PLATFORM_ICEBERG + "catalog.specifier.class";
Review Comment:
let's disambiguate that this is for the 'source' catalog. there may be a
different one for the destination side of the copy
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergCatalog.java:
##########
@@ -17,10 +17,23 @@
package org.apache.gobblin.data.management.copy.iceberg;
+import org.apache.iceberg.catalog.Catalog;
+
/**
* Any catalog from which to access {@link IcebergTable}s.
*/
public interface IcebergCatalog {
IcebergTable openTable(String dbName, String tableName);
+
+ /**
+ * Adding a sub interface to help us provide an association between {@link
Catalog} and {@link IcebergCatalog}.
+ * This helps us resolve to the Catalog type and its concrete implementation
class
Review Comment:
really this arises because the RTTI is needed within to provide access to
the `newTableOps`, which only certain `Catalog` derived classes open for public
access. that's the crux and worth noting specifically.
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergHiveCatalog.java:
##########
@@ -17,22 +17,54 @@
package org.apache.gobblin.data.management.copy.iceberg;
-import lombok.AllArgsConstructor;
-import lombok.extern.slf4j.Slf4j;
-
+import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.hive.HiveCatalog;
+import lombok.extern.slf4j.Slf4j;
+
/**
* Hive-Metastore-based {@link IcebergCatalog}.
*/
@Slf4j
-@AllArgsConstructor
public class IcebergHiveCatalog implements IcebergCatalog {
+
+ /**
+ * Ensures pairing between {@link IcebergCatalog} and its implementation
type i.e. {@link HiveCatalog} in this case.
+ * Unfortunately, {@link
org.apache.iceberg.BaseMetastoreCatalog}.newTableOps is protected.
+ * Hence it necessitates this abtraction to define and access {@link
IcebergTable}
+ */
+ public static class HiveSpecifier implements CatalogSpecifier {
+
+ public static final String HIVE_CATALOG_TYPE = "hive";
+ @Override
+ public Class<? extends Catalog> getCatalogClass() {
+ return HiveCatalog.class;
+ }
+
+ @Override
+ public Class<? extends IcebergCatalog> getIcebergCatalogClass() {
+ return IcebergHiveCatalog.class;
+ }
+
+ @Override
+ public String getCatalogType() {
+ return HIVE_CATALOG_TYPE;
+ }
+ }
+
// NOTE: specifically necessitates `HiveCatalog`, as
`BaseMetastoreCatalog.newTableOps` is `protected`!
private final HiveCatalog hc;
+ public IcebergHiveCatalog(Catalog catalog) {
+ if (catalog instanceof HiveCatalog) {
+ hc = (HiveCatalog) catalog;
+ } else {
+ throw new IllegalArgumentException("Incorrect Catalog Provided: Catalog
cannot resolve to HiveCatalog");
Review Comment:
you'll keep the person debugging in suspense!
i.e. let's tell them what class actually came in... ;)
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergHiveCatalog.java:
##########
@@ -17,22 +17,54 @@
package org.apache.gobblin.data.management.copy.iceberg;
-import lombok.AllArgsConstructor;
-import lombok.extern.slf4j.Slf4j;
-
+import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.hive.HiveCatalog;
+import lombok.extern.slf4j.Slf4j;
+
/**
* Hive-Metastore-based {@link IcebergCatalog}.
*/
@Slf4j
-@AllArgsConstructor
public class IcebergHiveCatalog implements IcebergCatalog {
+
+ /**
+ * Ensures pairing between {@link IcebergCatalog} and its implementation
type i.e. {@link HiveCatalog} in this case.
+ * Unfortunately, {@link
org.apache.iceberg.BaseMetastoreCatalog}.newTableOps is protected.
Review Comment:
good--clear and specific!
--
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]
