openinx commented on a change in pull request #1293:
URL: https://github.com/apache/iceberg/pull/1293#discussion_r469845803
##########
File path: flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java
##########
@@ -80,14 +85,16 @@ public FlinkCatalog(
String catalogName,
String defaultDatabase,
String[] baseNamespace,
- Catalog icebergCatalog,
+ CatalogLoader catalogLoader,
boolean cacheEnabled) {
super(catalogName, defaultDatabase);
- this.originalCatalog = icebergCatalog;
- this.icebergCatalog = cacheEnabled ? CachingCatalog.wrap(icebergCatalog) :
icebergCatalog;
+ this.originalCatalog = catalogLoader.loadCatalog(
+
HadoopUtils.getHadoopConfiguration(GlobalConfiguration.loadConfiguration()));
Review comment:
How about adding a `Configuration` argument in this current
constructor. IMO for `FlinkCatalog`, it shouldn't handle the logic about
configuration initialization. Moving the configuration loading to the
`FlinkCatalogFactory` sounds more reasonable. besides, unit test for
`FlinkCatalog` will be easy because it don't depend on how the flink will load
its configuration.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
