Core: Throw an exception if both catalog type and catalog-impl are set #3162

omarsmak · 2021-09-21T10:01:28Z

Currently if the user has set both configs in Hive iceberg.catalog.<catalog_name\>.type=hive|hadoop and iceberg.catalog.<catalog_name\>.catalog-impl=CustomCatalog, Iceberg will regardless use the type to perform some operations in HiveIcebergMetaHook that may introduce inconsistencies that are incompatible with the set custom catalog. For example, if type is set to hive and we have catalog-impl set to GlueCatalog,HiveIcebergMetaHook will perform operations based on hive catalog which are incompatible with the custom catalog GlueCatalog.
In order to mediate the issue, in this PR I am returning null in iceberg.catalog.<catalog_name\>.type in case we have iceberg.catalog.<catalog_name\>.catalog-impl is set in order to avoid such cases as described above.

Note: The behavior in regards to legacy configs is remained unchanged.

nastra

changes make sense to me, but I'm not a Hive expert, so maybe @rdblue or @pvary could review this one here as well?

mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java

mr/src/main/java/org/apache/iceberg/mr/Catalogs.java

pvary · 2021-09-21T11:47:09Z

For me if someone sets both iceberg.catalog.<catalog_name>.type and iceberg.catalog.<catalog_name>.catalog-impl is a wrong configuration. I think we should throw an exception instead of trying to define a precedence between the two.

What do you think?

omarsmak · 2021-09-21T13:07:41Z

For me if someone sets both iceberg.catalog.<catalog_name>.type and iceberg.catalog.<catalog_name>.catalog-impl is a wrong configuration. I think we should throw an exception instead of trying to define a precedence between the two.

What do you think?

I thought about it for a bit and I think it is actually a good idea as well to throw an exception, it has an advantage of being explicit to user and thus, the user is educated on how these config work. I will update the logic shortly. Thanks for the feedback!

omarsmak · 2021-09-21T14:03:02Z

@pvary I have changed the logic a bit to throw an exception instead. Please take a look at it and let me know what you think.

mr/src/main/java/org/apache/iceberg/mr/Catalogs.java

nastra · 2021-09-21T15:52:05Z

site/docs/hive.md

 | --------------------------------------------- | ------------------------------------------------------ |
-| iceberg.catalog.<catalog_name\>.type          | type of catalog: `hive` or `hadoop`                    |
-| iceberg.catalog.<catalog_name\>.catalog-impl  | catalog implementation, must not be null if type is null |
+| iceberg.catalog.<catalog_name\>.type          | type of catalog: `hive`, `hadoop` or empty if `catalog-impl` will be set.                  |


Now that we're throwing an exception, I think this change can be rolled back

I don't think it is necessary since this is the original intention to leave the type empty in case the catalog-impl will be set.

Non-blocking notes:

I agree that this can mostly be rolled back. I would say that type could be "hive or hadoop, or left unset if using a custom catalog".

kbendick · 2021-09-21T18:43:07Z

This brings up a larger issue (that is outside of the scope of this PR) that @kainoa21 ecently brought up recently on Slack and elsewhere, that we also experience when using GlueCatalog with Flink - that the custom catalog-impl's don't necessarily neatly fall into the Hadoop or Hive mindset.

In the case he brought up, he wanted to be able to use GlueCatalog in an AWS managed environment without having to have hadoop dependencies on the classpath. The only reason he needed them there was to satisfy some unused code constraints.

He felt that possibly it was a bit of a leaky abstraction in some places. His feeling was that maybe there should be a GlueCatalogFileIO or possibly even multiple possible GlueCatalogFileIO types - depending if one wanted to use Hive or not etc.

As this seems to be an offshoot of the same problem, tagging him and linking the issue for future reference: #3044

This is outside of the scope of this PR and I don't mean to block this PR, but this might be something we want to consider during V3 planning / as part of a larger scope of things as we do seem to be encountering some issues with what I'll call the well known "custom" catalogs? cc @rdblue (in case Jason doesn't come on Github as much) and @jackye1995 who was also working on this problem at the time.

We can continue this conversation elsewhere, sorry to thread-jack / PR-jack the discussion!

site/docs/hive.md

jackye1995 · 2021-09-22T06:23:47Z

mr/src/main/java/org/apache/iceberg/mr/Catalogs.java

    } else {
      String name = catalogName == null ? ICEBERG_DEFAULT_CATALOG_NAME : catalogName;
+
+      String catalogImpl = conf.get(InputFormatConfig.catalogPropertyConfigKey(


I think instead of doing the check here it's simpler to do the check in addCatalogPropertiesIfMissing before we set CatalogUtil.ICEBERG_CATALOG_TYPE to something. By doing that, you do not need to get the actual impl like this and you can just check catalogProperties.containsKey(CatalogProperties.CATALOG_IMPL). This also allows compatibility with legacy catalog impl Hadoop config key.

good point! I will change this then and add one more test for legacy configs

jackye1995 · 2021-09-22T06:25:51Z

I also agree that this should throw an exception instead of setting null. Could you update the title to reflect that?

I think we should also do a similar check in Spark and Flink code path to make sure we do not have the same issue, could you also add those checks?

omarsmak · 2021-09-22T11:04:25Z

I also agree that this should throw an exception instead of setting null. Could you update the title to reflect that?

I think we should also do a similar check in Spark and Flink code path to make sure we do not have the same issue, could you also add those checks?

I could do that, but would it make sense to keep this PR focused on Hive and raise a new one for Spark and Flink?

mr/src/main/java/org/apache/iceberg/mr/Catalogs.java

site/docs/hive.md

mr/src/main/java/org/apache/iceberg/mr/Catalogs.java

rdblue

@omarsmak, can the check be done in CatalogUtil instead of here? That way it would apply everywhere and not just for Hive.

omarsmak · 2021-10-11T15:20:47Z

@rdblue I have moved the logic to CatalogUtil instead of Catalog. This will work for both Spark and Hive. For Flink logic will be very similar to CatalogUtil but in FlinkCatalogFactory. If you think what I did here it makes sense for Hive and Spark, I will add Flink as well with the same logic .

core/src/main/java/org/apache/iceberg/CatalogUtil.java

core/src/test/java/org/apache/iceberg/TestCatalogUtil.java

core/src/main/java/org/apache/iceberg/CatalogUtil.java

omarsmak · 2021-10-11T17:01:10Z

@rdblue please take a look at it once again, I hope things now are in place :)

omarsmak · 2021-10-11T17:32:45Z

Some tests are failing, I will need to check tomorrow why they are failing

rdblue

Looks good now! I'll merge when tests are passing.

core/src/test/java/org/apache/iceberg/TestCatalogUtil.java

omarsmak · 2021-10-12T13:37:34Z

@rdblue CI now looks green.

rdblue · 2021-10-12T14:51:21Z

Thanks, @omarsmak!

…he#3162)

github-actions bot added docs MR labels Sep 21, 2021

nastra reviewed Sep 21, 2021

View reviewed changes

mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java Outdated Show resolved Hide resolved

mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java Outdated Show resolved Hide resolved

mr/src/main/java/org/apache/iceberg/mr/Catalogs.java Outdated Show resolved Hide resolved

nastra reviewed Sep 21, 2021

View reviewed changes

mr/src/main/java/org/apache/iceberg/mr/Catalogs.java Outdated Show resolved Hide resolved

pvary reviewed Sep 21, 2021

View reviewed changes

mr/src/main/java/org/apache/iceberg/mr/Catalogs.java Show resolved Hide resolved

pvary reviewed Sep 21, 2021

View reviewed changes

mr/src/main/java/org/apache/iceberg/mr/Catalogs.java Outdated Show resolved Hide resolved

nastra reviewed Sep 21, 2021

View reviewed changes

jackye1995 reviewed Sep 22, 2021

View reviewed changes

site/docs/hive.md Outdated Show resolved Hide resolved

jackye1995 reviewed Sep 22, 2021

View reviewed changes

omarsmak changed the title ~~Hive: Set catalog type to null if catalog-impl is set~~ Hive: Thrown an exception if both catalog type and catalog-impl are set Sep 22, 2021

omarsmak changed the title ~~Hive: Thrown an exception if both catalog type and catalog-impl are set~~ Hive: Throw an exception if both catalog type and catalog-impl are set Sep 22, 2021

rdblue reviewed Sep 22, 2021

View reviewed changes

mr/src/main/java/org/apache/iceberg/mr/Catalogs.java Outdated Show resolved Hide resolved

rdblue reviewed Sep 22, 2021

View reviewed changes

site/docs/hive.md Outdated Show resolved Hide resolved

rdblue reviewed Sep 22, 2021

View reviewed changes

mr/src/main/java/org/apache/iceberg/mr/Catalogs.java Outdated Show resolved Hide resolved

rdblue requested changes Sep 22, 2021

View reviewed changes

omarsmak force-pushed the fix-catalog-type-if-custom-set branch 2 times, most recently from 24680bc to 819374c Compare October 11, 2021 15:01

github-actions bot added the core label Oct 11, 2021

omarsmak force-pushed the fix-catalog-type-if-custom-set branch from 819374c to 947a952 Compare October 11, 2021 15:09

omarsmak changed the title ~~Hive: Throw an exception if both catalog type and catalog-impl are set~~ Core: Throw an exception if both catalog type and catalog-impl are set Oct 11, 2021

rdblue reviewed Oct 11, 2021

View reviewed changes

core/src/main/java/org/apache/iceberg/CatalogUtil.java Outdated Show resolved Hide resolved

rdblue reviewed Oct 11, 2021

View reviewed changes

core/src/test/java/org/apache/iceberg/TestCatalogUtil.java Outdated Show resolved Hide resolved

rdblue reviewed Oct 11, 2021

View reviewed changes

core/src/main/java/org/apache/iceberg/CatalogUtil.java Outdated Show resolved Hide resolved

omarsmak force-pushed the fix-catalog-type-if-custom-set branch from 947a952 to 45a301b Compare October 11, 2021 16:58

omarsmak requested a review from rdblue October 11, 2021 16:59

omarsmak force-pushed the fix-catalog-type-if-custom-set branch from 45a301b to 79f0d46 Compare October 11, 2021 17:49

rdblue approved these changes Oct 11, 2021

View reviewed changes

ajantha-bhat reviewed Oct 12, 2021

View reviewed changes

core/src/test/java/org/apache/iceberg/TestCatalogUtil.java Show resolved Hide resolved

omarsmak force-pushed the fix-catalog-type-if-custom-set branch from 79f0d46 to 0a3666c Compare October 12, 2021 10:28

Throw an exception if both, type and catalog-impl are set in CatalogUtil

90ecfdd

omarsmak force-pushed the fix-catalog-type-if-custom-set branch from 0a3666c to 90ecfdd Compare October 12, 2021 11:52

rdblue approved these changes Oct 12, 2021

View reviewed changes

rdblue merged commit c5eeb97 into apache:master Oct 12, 2021

omarsmak deleted the fix-catalog-type-if-custom-set branch October 12, 2021 14:52

omarsmak mentioned this pull request Oct 18, 2021

Flink: Fail if both Catalog type and catalog-impl are configured and add TestFlinkCatalogFactory tests #3308

Merged

kbendick pushed a commit to kbendick/iceberg that referenced this pull request Oct 27, 2021

Core: Fail if both Catalog type and catalog-impl are configured (apac…

fa75f7e

…he#3162)

RussellSpitzer pushed a commit to RussellSpitzer/iceberg that referenced this pull request Oct 29, 2021

Core: Fail if both Catalog type and catalog-impl are configured (apac…

01d26ba

…he#3162)

kbendick mentioned this pull request Oct 31, 2021

0.12.1 Cherry-Picks: Part 3 #3428

Merged

Core: Throw an exception if both catalog type and catalog-impl are set #3162

Core: Throw an exception if both catalog type and catalog-impl are set #3162

Uh oh!

Conversation

omarsmak commented Sep 21, 2021

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pvary commented Sep 21, 2021

Uh oh!

omarsmak commented Sep 21, 2021

Uh oh!

omarsmak commented Sep 21, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nastra Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

omarsmak Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

kbendick Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

kbendick commented Sep 21, 2021

Uh oh!

Uh oh!

jackye1995 Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

omarsmak Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

jackye1995 commented Sep 22, 2021

Uh oh!

omarsmak commented Sep 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

omarsmak commented Oct 11, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

omarsmak commented Oct 11, 2021

Uh oh!

omarsmak commented Oct 11, 2021

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

omarsmak commented Oct 12, 2021

Uh oh!

rdblue commented Oct 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants