jackye1995 commented on a change in pull request #1618:
URL: https://github.com/apache/iceberg/pull/1618#discussion_r518210569
##########
File path: flink/src/main/java/org/apache/iceberg/flink/CatalogLoader.java
##########
@@ -45,12 +45,22 @@
*/
Catalog loadCatalog();
- static CatalogLoader hadoop(String name, Configuration hadoopConf, String
warehouseLocation) {
- return new HadoopCatalogLoader(name, hadoopConf, warehouseLocation);
+ static CatalogLoader hadoop(
+ String name,
+ Configuration hadoopConf,
+ String warehouseLocation,
+ Map<String, String> properties) {
+ return new HadoopCatalogLoader(name, hadoopConf, warehouseLocation,
properties);
Review comment:
I only see the `CatalogLoader.hadoop` and `CatalogLoader.hive` used in a
single place `FlinkCatalogFactory.createCatalogLoader`, that is why I directly
changed the class signature. I don't know if there is any benefit in keeping
the old ones.
Changing the signature to `CatalogLoader.hadoop(String name, Map<String,
String> properties, Configuration conf)` looks like a good idea that simplifies
the code, let me do that first.
(speaking of this, I should also add null check for the properties map and
also give a fixed serialization id for those classes because Flink serializes
the catalog loader)
----------------------------------------------------------------
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]
