-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-28658: Iceberg: Implement REST Catalog HMS Client #5995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ade3343 to
35b7879
Compare
...eberg-catalog/src/main/java/org/apache/iceberg/hive/HiveIcebergRESTCatalogClientAdapter.java
Outdated
Show resolved
Hide resolved
...eberg-catalog/src/main/java/org/apache/iceberg/hive/HiveIcebergRESTCatalogClientAdapter.java
Outdated
Show resolved
Hide resolved
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/Catalogs.java
Outdated
Show resolved
Hide resolved
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/Catalogs.java
Outdated
Show resolved
Hide resolved
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/InputFormatConfig.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hadoop/hive/metastore/client/builder/HiveMetaStoreClientBuilder.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hadoop/hive/metastore/client/builder/HiveMetaStoreClientBuilder.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hadoop/hive/metastore/client/builder/HiveMetaStoreClientBuilder.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/hive/metastore/client/builder/HiveMetaStoreClientBuilderException.java
Outdated
Show resolved
Hide resolved
...tore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Outdated
Show resolved
Hide resolved
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HMSTablePropertyHelper.java
Outdated
Show resolved
Hide resolved
35b7879 to
f66c605
Compare
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/Catalogs.java
Outdated
Show resolved
Hide resolved
| if (StringUtils.isEmpty(catalogType) || CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE.equals(catalogType)) { | ||
| return new HiveIcebergMetaHook(conf); | ||
| } else { | ||
| conf.set(ConfigProperties.LOCK_HIVE_ENABLED, "false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not part of this PR, however, I am not sure if we are not breaking transaction isolation for non-native catalogs.
HiveTableOperations.doCommit() is doing an atomic METADATA_LOCATION_PROP check & update in HMS backend DB. If location was changed by concurrent transaction - we rollback.
How is that supposed to work in case of Rest or Hadoop catalogs?
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/CatalogUtils.java
Outdated
Show resolved
Hide resolved
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/BaseHiveIcebergMetaHook.java
Outdated
Show resolved
Hide resolved
e899198 to
6f7b6df
Compare
6f7b6df to
77f2b99
Compare
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/CatalogUtils.java
Outdated
Show resolved
Hide resolved
|
+1 |
|



What changes were proposed in this pull request?
Hive Iceberg RESTCatalog client adaptor.
HMSClient can be backed by this adaptor and this allows Hive to work directly with Iceberg REST catalogs.
Why are the changes needed?
To enable HMSClient work with Iceberg RESTCatalog without HMS.
Does this PR introduce any user-facing change?
Yes, users can make Hive work directly with Iceberg REST catalogs.
How was this patch tested?
New unit and integration tests.
Note: This PR builds on the work originally started by @zratkai in PR #5628. The goal of this PR is to bring that work to completion by fixing HMS REST Client implementation, handling all open issues brought up in code reviews, adapting the implementation to the latest hive changes, and development of Java-based integration tests and q-tests.