Skip to content

Unbind metastore config from iceberg and delta lake modules#12595

Merged
findepi merged 2 commits intotrinodb:masterfrom
homar:homar/remove_metastore_config_from_iceberg_module
Jun 3, 2022
Merged

Unbind metastore config from iceberg and delta lake modules#12595
findepi merged 2 commits intotrinodb:masterfrom
homar:homar/remove_metastore_config_from_iceberg_module

Conversation

@homar
Copy link
Copy Markdown
Member

@homar homar commented May 30, 2022

Description

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label May 30, 2022
@homar homar force-pushed the homar/remove_metastore_config_from_iceberg_module branch from cf30d2d to 9ebbe2b Compare May 30, 2022 09:37
@homar homar force-pushed the homar/remove_metastore_config_from_iceberg_module branch 2 times, most recently from a5d8d5d to 367c2e1 Compare May 30, 2022 12:16
findepi
findepi requested changes May 30, 2022
View reviewed changes
...in/trino-hive/src/main/java/io/trino/plugin/hive/metastore/alluxio/AlluxioHiveMetastore.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi May 30, 2022

Choose a reason for hiding this comment

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

update the exception message

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi May 30, 2022

Choose a reason for hiding this comment

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

here too

...c/main/java/io/trino/plugin/hive/metastore/glue/DefaultGlueMetastoreTableFilterProvider.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi May 30, 2022

Choose a reason for hiding this comment

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

and maybe here

...o-delta-lake/src/main/java/io/trino/plugin/deltalake/metastore/DeltaLakeMetastoreModule.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi May 30, 2022

Choose a reason for hiding this comment

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

in Delta this makes little sense to hide Delta tables.

maybe fix Iceberg and Delta in one go?

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/HiveMetastoreModule.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi May 30, 2022

Choose a reason for hiding this comment

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

this should come from the config

    @HideDeltaLakeTables
    @Singleton
    @Provides
    public boolean hideDeltaLakeTables(HiveMetastoreConfig hiveMetastoreConfig)
    {
        return hiveMetastoreConfig.isHideDeltaLakeTables();

Copy link
Copy Markdown
Member Author

@homar homar May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok I considered this but I was not sure about it

@homar homar force-pushed the homar/remove_metastore_config_from_iceberg_module branch 2 times, most recently from 43673c2 to d5f9248 Compare May 30, 2022 15:58
@homar homar marked this pull request as ready for review May 30, 2022 19:32
@homar homar changed the title [WIP] Unbind metastore config from iceberg module Unbind metastore config from iceberg module May 30, 2022
@homar
Copy link
Copy Markdown
Member Author

homar commented May 31, 2022

@findepi do you mind taking another look ?

findepi
findepi approved these changes Jun 2, 2022
View reviewed changes
...rc/test/java/io/trino/plugin/deltalake/metastore/TestDeltaLakeMetastoreAccessOperations.java Outdated
new NodeVersion("testversion"),
hdfsEnvironment,
new HiveMetastoreConfig(),
new HiveMetastoreConfig().isHideDeltaLakeTables(),
Copy link
Copy Markdown
Member

@findepi findepi Jun 2, 2022

Choose a reason for hiding this comment

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

false (we're in Delta)

...lake/src/test/java/io/trino/plugin/deltalake/metastore/TestDeltaLakeMetastoreStatistics.java Outdated
new NodeVersion("test_version"),
hdfsEnvironment,
new HiveMetastoreConfig(),
new HiveMetastoreConfig().isHideDeltaLakeTables(),
Copy link
Copy Markdown
Member

@findepi findepi Jun 2, 2022

Choose a reason for hiding this comment

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

false (we're in Delta)

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveFileMetastore.java Outdated
Comment on lines +41 to +42
.setHideDeltaLakeTables(true)
.isHideDeltaLakeTables(),
Copy link
Copy Markdown
Member

@findepi findepi Jun 2, 2022

Choose a reason for hiding this comment

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

just pass true

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java Outdated
{
binder.bind(IcebergTransactionManager.class).in(Scopes.SINGLETON);
configBinder(binder).bindConfig(IcebergConfig.class);
configBinder(binder).bindConfig(HiveMetastoreConfig.class);
Copy link
Copy Markdown
Member

@findepi findepi Jun 2, 2022

Choose a reason for hiding this comment

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

I think you should be able to just do the same in Delta, right?
you did all the Delta changes anyway.

Just update the commit message and PR title when doing so.

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMetadataListing.java Outdated
new NodeVersion("test_version"),
hdfsEnvironment,
new HiveMetastoreConfig(),
new HiveMetastoreConfig().isHideDeltaLakeTables(),
Copy link
Copy Markdown
Member

@findepi findepi Jun 2, 2022

Choose a reason for hiding this comment

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

false (as in IcebergHiveMetastoreCatalogModule)

...rino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMetastoreAccessOperations.java Outdated
new NodeVersion("testversion"),
hdfsEnvironment,
new HiveMetastoreConfig(),
new HiveMetastoreConfig().isHideDeltaLakeTables(),
Copy link
Copy Markdown
Member

@findepi findepi Jun 2, 2022

Choose a reason for hiding this comment

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

false (as in IcebergHiveMetastoreCatalogModule)

.../trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergTableWithCustomLocation.java Outdated
new NodeVersion("testversion"),
hdfsEnvironment,
new HiveMetastoreConfig(),
new HiveMetastoreConfig().isHideDeltaLakeTables(),
Copy link
Copy Markdown
Member

@findepi findepi Jun 2, 2022

Choose a reason for hiding this comment

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

false (as in IcebergHiveMetastoreCatalogModule)

...rino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergTableWithExternalLocation.java Outdated
new NodeVersion("testversion"),
hdfsEnvironment,
new HiveMetastoreConfig(),
new HiveMetastoreConfig().isHideDeltaLakeTables(),
Copy link
Copy Markdown
Member

@findepi findepi Jun 2, 2022

Choose a reason for hiding this comment

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

false (as in IcebergHiveMetastoreCatalogModule)

...g/src/test/java/io/trino/plugin/iceberg/optimizer/TestConnectorPushdownRulesWithIceberg.java Outdated
new NodeVersion("testversion"),
environment,
new HiveMetastoreConfig(),
new HiveMetastoreConfig().isHideDeltaLakeTables(),
Copy link
Copy Markdown
Member

@findepi findepi Jun 2, 2022

Choose a reason for hiding this comment

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

false (this is for iceberg)

@findepi findepi mentioned this pull request Jun 2, 2022
Merged
@homar homar force-pushed the homar/remove_metastore_config_from_iceberg_module branch 2 times, most recently from 5e95e3a to 02d2029 Compare June 2, 2022 14:54
findepi
findepi approved these changes Jun 3, 2022
View reviewed changes
...rg/src/main/java/io/trino/plugin/iceberg/catalog/file/IcebergFileMetastoreCatalogModule.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Jun 3, 2022

Choose a reason for hiding this comment

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

Use TestingIcebergPlugin.HIDE_DELTA_LAKE_TABLES (of course requires moving the constant)

...erg/src/main/java/io/trino/plugin/iceberg/catalog/hms/IcebergHiveMetastoreCatalogModule.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Jun 3, 2022

Choose a reason for hiding this comment

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

Use TestingIcebergPlugin.HIDE_DELTA_LAKE_TABLES

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestingIcebergPlugin.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Jun 3, 2022

Choose a reason for hiding this comment

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

maybe "HIDE_DELTA_LAKE_TABLES_IN_ICEBERG"
so that staticly imported remains unambiguous

(Hive, Iceberg and Delta connectors have overlapping classpaths, so we need to be extra careful)

@homar homar force-pushed the homar/remove_metastore_config_from_iceberg_module branch from 02d2029 to 305608a Compare June 3, 2022 10:48
@findepi findepi merged commit 4aacac2 into trinodb:master Jun 3, 2022
@findepi findepi added the no-release-notes This pull request does not require release notes entry label Jun 3, 2022
@github-actions github-actions bot added this to the 384 milestone Jun 3, 2022
@colebow colebow mentioned this pull request Jun 3, 2022
Closed
@homar homar changed the title Unbind metastore config from iceberg module Unbind metastore config from iceberg and delta lake modules Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findepi findepi findepi approved these changes

Assignees

No one assigned

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Milestone

384

Development

Successfully merging this pull request may close these issues.

2 participants

@homar @findepi