Add support for HTTP transport in thrift metastore client #17925

vihangk1 · 2023-06-15T20:44:39Z

Description

This PR adds support for using HTTP as the transport for the thrift metastore client. In this mode the hive.metastore.uri can support a http(s) URL to the metastore service. Since each thrift API is a POST request to the the metastore endpoint, this PR also adds support for adding a HTTP bearer token which must be configured to authenticate the metastore client to the metastore server. The token can be configured using a configuration hive.metastore.http.client.bearer-token. To support Databricks Unity Catalog's HMS API interface, the configuration hive.metastore.http.client.additional-headers must be set to X-Databricks-Unity-Catalog-Name=<catalog_name> where the catalog_name is the name of the catalog object in Databricks Unity Catalog.

Additional context and related issues

Thrift has support HTTP based transport for a long time. Ref https://github.com/apache/thrift/blob/master/lib/javame/src/org/apache/thrift/transport/THttpClient.java. We have used the http mode for thrift on various services in other projects like HiveServer2 in Apache Hive and Apache Spark for a while now.

More recently, Apache Hive also added support for Hive Metastore server to use HTTP mode.
https://issues.apache.org/jira/browse/HIVE-21456

This PR modifies the logic in DefaultThriftMetastoreClientFactory to instantiate THttpClient for the transport when creating the thrift metastore client.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

Add HTTP support for the thrift metastore client.

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

Fixes #17865

cla-bot · 2023-06-15T20:44:41Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2023-06-15T21:32:40Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

martint · 2023-06-20T18:06:38Z

@cla-bot check

cla-bot · 2023-06-20T18:06:41Z

The cla-bot has been summoned, and re-checked this pull request!

...src/main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java

...in/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreConfig.java

...-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/MockThriftHttpMetastoreServer.java

...-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHttpMetastoreClient.java

cla-bot · 2023-06-28T00:59:35Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Vihang Karajgaonkar.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

vihangk1 · 2023-06-29T16:33:51Z

Hi @electrum Would you like to take another look? I think I addressed all your review comments.

electrum · 2023-07-05T22:32:44Z

Can you squash all of the commits?

vihangk1 · 2023-07-06T17:55:13Z

@electrum Squashed all commits into one and force pushed.

electrum

Thanks for updating this. Looking at the config closer, rather than adding hive.metastore.thrift.transport-mode, we should use URI from hive.metastore.uri to determine the protocol. That's actually why we used a URI in the first place, rather than just a host/port. This will also allow mixing raw and HTTP endpoints.

vihangk1 · 2023-07-10T17:18:50Z

I think that makes sense. Let me update the PR to derive the HTTP mode directly from the URI scheme.

vihangk1 · 2023-07-10T17:45:34Z

Thanks for updating this. Looking at the config closer, rather than adding hive.metastore.thrift.transport-mode, we should use URI from hive.metastore.uri to determine the protocol. That's actually why we used a URI in the first place, rather than just a host/port. This will also allow mixing raw and HTTP endpoints.

@electrum Ah, I remember now why I had to introduce a http mode config. The interface to create the metastore client takes a HostAndPort unfortunately here

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClientFactory.java

Line 23 in 15019f0

    
           ThriftMetastoreClient create(HostAndPort address, Optional<String> delegationToken)

Due to this we don't have the URI scheme to decide if we should use a http client or a binary client. I can change the interface to include uri but then the changes trickle down to other files as well and the scope of the PR increases. I am okay to do that but just wanted to make sure that I am understanding this right before getting into the changes.

vihangk1 · 2023-07-12T07:01:48Z

I updated the PR to reuse the metastore URI scheme to determine if we should use the http thrift transport. I added a separate commit so that we can see what all changes are needed just to reuse the scheme from the metastore uri directly.

ebyhr · 2023-10-27T08:52:47Z

Please fix CI failure https://github.com/trinodb/trino/actions/runs/6664185107/job/18111804372?pr=19085

vihangk1 · 2023-10-28T05:52:52Z

https://github.com/trinodb/trino/actions/runs/6664185107/job/18111804372?pr=19085

I think the product test in the link above is failing due to some infrastructure issue. It is failing because it cannot find the jdbc driver to connect to the databricks cluster. The pt works for me when I run locally on my environment.

ebyhr · 2023-10-30T06:31:08Z

The product test is still failing https://github.com/trinodb/trino/actions/runs/6688269641/job/18170433430?pr=19085

vihangk1 · 2023-10-30T16:49:29Z

https://github.com/trinodb/trino/actions/runs/6688269641/job/18170433430?pr=19085

I saw the error. those tests are failing due to cluster not being found (see below). Can we check if the databricks cluster terminated during that time?

RESOURCE_DOES_NOT_EXIST: No cluster found matching: 0907-043916-ahg9aqt3

plugin/trino-hive/pom.xml

ebyhr · 2023-11-02T09:09:24Z

...in/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreModule.java

+        ThriftMetastoreConfig metastoreConfig = buildConfigObject(ThriftMetastoreConfig.class);
+        ThriftHttpMetastoreConfig httpMetastoreConfig = buildConfigObject(ThriftHttpMetastoreConfig.class);
+
+        if (!staticMetastoreConfig.getMetastoreUris().isEmpty()) {


When does getMetastoreUris return empty? It would be nice to deny if it's unexpected:

checkArgument(!staticMetastoreConfig.getMetastoreUris().isEmpty(), "...");

If I remember correctly, I think there were some tests which were failing which caused me add this line. But I cannot find which ones were those. I have added this precondition check for now to see if the CI reveals those failing tests again.

When I add a checkArgument condition here, I see test failures for TestHiveConnectoryFactory which expect a different error message asserted in StaticTokenAwareMetastoreClientFactory

I changed the precondition check to checkNotNull for the metastoreUris. If the metastoreUris is empty the validateThriftHttpConfiguration becomes no-op and the higher layers like StaticTokenAwareMetastoreClientFactory will assert the metastoreUris are non-empty.

...-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHttpMetastoreClient.java

...main/java/io/trino/tests/product/launcher/env/environment/EnvMultinodeDatabricksHttpHms.java

...src/main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java

vihangk1 · 2023-11-09T18:36:50Z

rebased and resolved conflicts.

vihangk1 · 2023-11-14T23:49:36Z

The CI failure looks unrelated. It failed due to "Terminating due to java.lang.OutOfMemoryError: Java heap space"

shashanksinghfd · 2023-11-18T17:25:02Z

Thanks @vihangk1 for keeping the PR alive even after 5 months of to and fro , in true spirit of open source :)
thanks maintainers too, hope this gets merged .

sajjoseph · 2024-01-11T13:21:25Z

Any updates? It will be great if we can get this merged so that Trino can integrate Unity catalog.

findinpath · 2024-01-11T13:26:07Z

Adding here a snippet from a Slack conversation with @vihangk1

public class ThriftMetastoreModule
        extends AbstractConfigurationAwareModule
{
    @Override
    protected void setup(Binder binder)
    {



        binder.bind(TokenAwareMetastoreClientFactory.class).to(StaticTokenAwareMetastoreClientFactory.class).in(Scopes.SINGLETON);
        configBinder(binder).bindConfig(StaticMetastoreConfig.class);



        StaticMetastoreConfig staticMetastoreConfig = buildConfigObject(StaticMetastoreConfig.class);
        boolean hasHttpMetastore = staticMetastoreConfig.getMetastoreUris().stream()
                .anyMatch(uri -> "http".equalsIgnoreCase(uri.getScheme()));
        boolean hasHttpsMetastore = staticMetastoreConfig.getMetastoreUris().stream()
                .anyMatch(uri -> "https".equalsIgnoreCase(uri.getScheme()));

        if (hasHttpMetastore || hasHttpsMetastore) {
            configBinder(binder).bindConfig(ThriftHttpMetastoreConfig.class);
            // bind the factory
        } else {
            configBinder(binder).bindConfig(ThriftMetastoreConfig.class);
            OptionalBinder.newOptionalBinder(binder, ThriftMetastoreClientFactory.class)
                    .setDefault().to(DefaultThriftMetastoreClientFactory.class).in(Scopes.SINGLETON);

            // ....
        }

I was suggesting him to separate what is related to thrift http configuration in a separate class which would be bound only if we're dealing with a http metastore.

@vihangk1 do you have any free cycles in the near future to continue with this PR?

vihangk1 · 2024-01-12T07:11:04Z

Adding here a snippet from a Slack conversation with @vihangk1

public class ThriftMetastoreModule
        extends AbstractConfigurationAwareModule
{
    @Override
    protected void setup(Binder binder)
    {



        binder.bind(TokenAwareMetastoreClientFactory.class).to(StaticTokenAwareMetastoreClientFactory.class).in(Scopes.SINGLETON);
        configBinder(binder).bindConfig(StaticMetastoreConfig.class);



        StaticMetastoreConfig staticMetastoreConfig = buildConfigObject(StaticMetastoreConfig.class);
        boolean hasHttpMetastore = staticMetastoreConfig.getMetastoreUris().stream()
                .anyMatch(uri -> "http".equalsIgnoreCase(uri.getScheme()));
        boolean hasHttpsMetastore = staticMetastoreConfig.getMetastoreUris().stream()
                .anyMatch(uri -> "https".equalsIgnoreCase(uri.getScheme()));

        if (hasHttpMetastore || hasHttpsMetastore) {
            configBinder(binder).bindConfig(ThriftHttpMetastoreConfig.class);
            // bind the factory
        } else {
            configBinder(binder).bindConfig(ThriftMetastoreConfig.class);
            OptionalBinder.newOptionalBinder(binder, ThriftMetastoreClientFactory.class)
                    .setDefault().to(DefaultThriftMetastoreClientFactory.class).in(Scopes.SINGLETON);

            // ....
        }

I was suggesting him to separate what is related to thrift http configuration in a separate class which would be bound only if we're dealing with a http metastore.

@vihangk1 do you have any free cycles in the near future to continue with this PR?

Thanks @sajjoseph and @findinpath for your comments. I could not find enough time to investigate the approach which @findinpath suggested above in the last month. I made some progress on that and I just need to address some test failures after my changes. I will send out a PR update by this weekend with the suggested approach.

vihangk1 · 2024-02-05T01:51:55Z

Closing this PR in favor of #20371

github-actions bot added hive Hive connector tests:hive labels Jun 15, 2023

vihangk1 closed this Jun 15, 2023

vihangk1 reopened this Jun 15, 2023

vihangk1 force-pushed the master_thrift-http branch from 1d19c77 to 47ca558 Compare June 15, 2023 21:32

cla-bot bot added the cla-signed label Jun 20, 2023

vihangk1 self-assigned this Jun 20, 2023

electrum reviewed Jun 22, 2023

View reviewed changes

cla-bot bot removed the cla-signed label Jun 28, 2023

vihangk1 force-pushed the master_thrift-http branch from 75f7992 to 556adc5 Compare June 28, 2023 01:15

cla-bot bot added the cla-signed label Jun 28, 2023

vihangk1 force-pushed the master_thrift-http branch 2 times, most recently from f44a1bf to 40eb59b Compare June 29, 2023 02:40

github-actions bot added the hudi Hudi connector label Jun 29, 2023

vihangk1 force-pushed the master_thrift-http branch from f7d82b7 to 1174392 Compare July 6, 2023 17:53

electrum reviewed Jul 7, 2023

View reviewed changes

saurabh279 mentioned this pull request Jul 9, 2023

Add a hive connector property to disable SSL hostname verification #18194

Closed

github-actions bot added iceberg Iceberg connector delta-lake Delta Lake connector labels Jul 13, 2023

ebyhr force-pushed the master_thrift-http branch from 191c15e to 9302b9a Compare October 27, 2023 07:23

vihangk1 force-pushed the master_thrift-http branch 2 times, most recently from 9d60718 to 69a386e Compare October 29, 2023 06:02

ebyhr force-pushed the master_thrift-http branch from 9bc89ec to 64f1e65 Compare November 2, 2023 08:35

ebyhr reviewed Nov 2, 2023

View reviewed changes

vihangk1 force-pushed the master_thrift-http branch 2 times, most recently from 502e7c8 to 9ffa0b3 Compare November 7, 2023 06:46

ebyhr reviewed Nov 7, 2023

View reviewed changes

...src/main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java Outdated Show resolved Hide resolved

vihangk1 force-pushed the master_thrift-http branch 2 times, most recently from 28eea16 to 8ce2605 Compare November 9, 2023 18:36

vihangk1 force-pushed the master_thrift-http branch 2 times, most recently from 558dbfe to 7952c33 Compare November 10, 2023 23:04

vihangk1 added 3 commits November 14, 2023 11:30

Add support for HTTP transport in thrift metastore client

c18d06b

Ignore empty strings for database locations

dc383ca

review comments

a5ade38

vihangk1 force-pushed the master_thrift-http branch from 7952c33 to a5ade38 Compare November 14, 2023 19:36

ebyhr requested a review from electrum November 15, 2023 01:40

ebyhr mentioned this pull request Jan 14, 2024

Add support for HTTP transport in thrift metastore client #20371

Merged

vihangk1 closed this Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for HTTP transport in thrift metastore client #17925

Add support for HTTP transport in thrift metastore client #17925

vihangk1 commented Jun 15, 2023 •

edited

Loading

cla-bot bot commented Jun 15, 2023

cla-bot bot commented Jun 15, 2023

martint commented Jun 20, 2023

cla-bot bot commented Jun 20, 2023

cla-bot bot commented Jun 28, 2023

vihangk1 commented Jun 29, 2023

electrum commented Jul 5, 2023

vihangk1 commented Jul 6, 2023

electrum left a comment

vihangk1 commented Jul 10, 2023

vihangk1 commented Jul 10, 2023 •

edited

Loading

vihangk1 commented Jul 12, 2023

ebyhr commented Oct 27, 2023 •

edited

Loading

vihangk1 commented Oct 28, 2023

ebyhr commented Oct 30, 2023

vihangk1 commented Oct 30, 2023

ebyhr Nov 2, 2023

vihangk1 Nov 6, 2023

vihangk1 Nov 7, 2023

vihangk1 Nov 7, 2023

vihangk1 commented Nov 9, 2023

vihangk1 commented Nov 14, 2023

shashanksinghfd commented Nov 18, 2023

sajjoseph commented Jan 11, 2024

findinpath commented Jan 11, 2024

vihangk1 commented Jan 12, 2024

vihangk1 commented Feb 5, 2024

Add support for HTTP transport in thrift metastore client #17925

Add support for HTTP transport in thrift metastore client #17925

Conversation

vihangk1 commented Jun 15, 2023 • edited Loading

Description

Additional context and related issues

Release notes

cla-bot bot commented Jun 15, 2023

cla-bot bot commented Jun 15, 2023

martint commented Jun 20, 2023

cla-bot bot commented Jun 20, 2023

cla-bot bot commented Jun 28, 2023

vihangk1 commented Jun 29, 2023

electrum commented Jul 5, 2023

vihangk1 commented Jul 6, 2023

electrum left a comment

Choose a reason for hiding this comment

vihangk1 commented Jul 10, 2023

vihangk1 commented Jul 10, 2023 • edited Loading

vihangk1 commented Jul 12, 2023

ebyhr commented Oct 27, 2023 • edited Loading

vihangk1 commented Oct 28, 2023

ebyhr commented Oct 30, 2023

vihangk1 commented Oct 30, 2023

ebyhr Nov 2, 2023

Choose a reason for hiding this comment

vihangk1 Nov 6, 2023

Choose a reason for hiding this comment

vihangk1 Nov 7, 2023

Choose a reason for hiding this comment

vihangk1 Nov 7, 2023

Choose a reason for hiding this comment

vihangk1 commented Nov 9, 2023

vihangk1 commented Nov 14, 2023

shashanksinghfd commented Nov 18, 2023

sajjoseph commented Jan 11, 2024

findinpath commented Jan 11, 2024

vihangk1 commented Jan 12, 2024

vihangk1 commented Feb 5, 2024

vihangk1 commented Jun 15, 2023 •

edited

Loading

vihangk1 commented Jul 10, 2023 •

edited

Loading

ebyhr commented Oct 27, 2023 •

edited

Loading