Skip to content
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

Use createTestingFileHiveMetastore method in tests #15468

Merged
merged 2 commits into from
Dec 20, 2022

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Dec 20, 2022

Description

Use createTestingFileHiveMetastore method in tests

Release notes

(x) This is not user-visible or docs only and no release notes are required.

@ebyhr ebyhr added the no-release-notes This pull request does not require release notes entry label Dec 20, 2022
@cla-bot cla-bot bot added the cla-signed label Dec 20, 2022
@ebyhr ebyhr force-pushed the ebi/iceberg-cleanup-file-metastore-test branch 3 times, most recently from b693a21 to 528fa9e Compare December 20, 2022 06:09
@ebyhr
Copy link
Member Author

ebyhr commented Dec 20, 2022

CI hit #14248

@ebyhr ebyhr requested review from findepi and findinpath December 20, 2022 08:24
HiveMetastore metastore = new FileHiveMetastore(
new NodeVersion("testversion"),
HDFS_ENVIRONMENT,
HIDE_DELTA_LAKE_TABLES_IN_ICEBERG,
Copy link
Contributor

Choose a reason for hiding this comment

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

potential follow-up: HIDE_DELTA_LAKE_TABLES_IN_ICEBERG is used now only in:

  • IcebergHiveMetastoreCatalogModule
  • IcebergFileMetastoreCatalogModule

We can probably drop this constant and replace it with false in the bindings.

new FileHiveMetastoreConfig()
.setCatalogDirectory(baseDir.toURI().toString())
.setMetastoreUser("test"));
HiveMetastore hiveMetastore = createTestingFileHiveMetastore(baseDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

can be inlined in the subsequent line where metastore is being initialized.

new FileHiveMetastoreConfig()
.setCatalogDirectory(dataDirectory.toFile().toURI().toString())
.setMetastoreUser("test"));
HiveMetastore metastore = createTestingFileHiveMetastore(dataDirectory.toFile());
Copy link
Contributor

Choose a reason for hiding this comment

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

Inline in the next statement

@@ -36,6 +26,7 @@

import java.nio.file.Path;

import static io.trino.plugin.hive.metastore.file.FileHiveMetastore.createTestingFileHiveMetastore;
Copy link
Contributor

Choose a reason for hiding this comment

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

pre-existing/ follow-up: This class tests the situation where the file metastore is being shared and not the hive metastore. I'd advocate for renaming the class to TestSharedFileMetastore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please feel free to send the follow-up PR.

@findinpath findinpath requested a review from pajaks December 20, 2022 09:05
@ebyhr ebyhr force-pushed the ebi/iceberg-cleanup-file-metastore-test branch from b1d8ba5 to 3fd90af Compare December 20, 2022 09:11
@ebyhr ebyhr force-pushed the ebi/iceberg-cleanup-file-metastore-test branch from 3fd90af to cc40061 Compare December 20, 2022 11:58
@ebyhr ebyhr merged commit e07e822 into trinodb:master Dec 20, 2022
@ebyhr ebyhr deleted the ebi/iceberg-cleanup-file-metastore-test branch December 20, 2022 22:57
@github-actions github-actions bot added this to the 404 milestone Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

3 participants