harshm-dev commented on a change in pull request #3326:
URL: https://github.com/apache/iceberg/pull/3326#discussion_r733462426
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -208,23 +201,14 @@ public static Catalog loadCatalog(
*/
public static Catalog buildIcebergCatalog(String name, Map<String, String>
options, Configuration conf) {
String catalogImpl = options.get(CatalogProperties.CATALOG_IMPL);
+ String catalogType = options.get(CatalogProperties.CATALOG_TYPE);
+ Preconditions.checkArgument(catalogImpl == null || catalogType == null,
+ "Cannot create catalog %s, both type and catalog-impl are set:
type=%s, catalog-impl=%s",
Review comment:
+1 @omarsmak
Q - type is an existing catalog configuration requirement
The 'type' is an existing spark conf -
https://iceberg.apache.org/spark-configuration/#catalog-configuration. Not
introducing anything new here.
Q - I think people need to put type = iceberg on their Flink catalogs. Is
this going to cause confusion at all
Consistent conf is always better :-). But changing an existing conf would
cause regressions. May be we should try to do that through a deprecation path.
For now, I'll keep it out of scope of this PR.
I'll attempt this change FlinkCatalogFactory in a different PR.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
