Skip to content

Consolidate iceberg native-mode into the catalog type#17233

Merged
zhenxiao merged 1 commit intoprestodb:masterfrom
ChunxuTang:iceberg-native
Jan 27, 2022
Merged

Consolidate iceberg native-mode into the catalog type#17233
zhenxiao merged 1 commit intoprestodb:masterfrom
ChunxuTang:iceberg-native

Conversation

@ChunxuTang
Copy link
Copy Markdown
Member

@ChunxuTang ChunxuTang commented Jan 27, 2022

As when the iceberg catalog type is HADOOP, the connector won't depend on the Hive metastore anymore, aka, in the native mode, we can safely remove the native mode from the iceberg connector config and only use the catalog type to determine whether it's in the native mode.

If the catalog type is HIVE, we use Hive metastore and it's in the non-native mode.
If the catalog type is HADOOP, we use HDFS to store both data and metadata and it's in the native mode.

Test plan - Unit test and integration test

== RELEASE NOTE ==

Iceberg Changes
* Remove the native-mode iceberg config. Use catalog.type instead.

@ChunxuTang
Copy link
Copy Markdown
Member Author

Hi @beinan and @zhenxiao, could you have a review of this PR? Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: using switch? your call

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I refactored the if/else if statements to switch.

Comment on lines 72 to 76
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be out of scope of this PR, I'm thinking make this getIcebergTable logic to be a utility method or in polymorphism

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree! I created a utility function to get the iceberg table.
Actually there are some other places such as IcebergMetadata and IcebergNativeMetadata where we could do some optimization by creating new abstraction layers. As this PR just focuses on merging the native-mode config into the catalog type, I'll send some follow-up PRs for these new abstraction layers.

Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good @ChunxuTang could you please add release notes? This PR changes the Iceberg config, removes iceberg.native-mode, let's make corresponding doc changes

@ChunxuTang
Copy link
Copy Markdown
Member Author

@zhenxiao Thanks for the review!
I updated the release note accordingly. Regarding the doc, I noticed that the doc is not updated since the support of iceberg native catalogs (#16612). I'll send a separate PR to update the doc.

@zhenxiao zhenxiao merged commit f91d2f6 into prestodb:master Jan 27, 2022
@ChunxuTang ChunxuTang deleted the iceberg-native branch January 27, 2022 17:11
@neeradsomanchi neeradsomanchi mentioned this pull request Feb 8, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants