Add ability to disable batch fetch tables and views from HMS by huberty89 · Pull Request #18274 · trinodb/trino

huberty89 · 2023-07-13T10:26:13Z

Description

Look like previously introducing batching is not supported well by old versions of HMS, more in a linked issue

Additional context and related issues

Fixes #18111 introduced after #17127

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:

# Hive
* Add config property for disabling batch metadata fetch from HMS. ({issue}`18111`)

huberty89 · 2023-07-13T10:28:08Z

Not sure where should I add a test to verify if setting new config property make an effect + if current place is the right place.

kokosing

Some test?

huberty89 · 2023-07-27T10:33:12Z

Some test?

Added TestThriftBatchFetchSwitch

Praveen2112 · 2023-07-27T10:35:23Z

Apart from disabling it - can we have a fallback mechanism if the query failed during bulkload api ?

huberty89 · 2023-07-27T11:09:17Z

Apart from disabling it - can we have a fallback mechanism if the query failed during bulkload api ?

@Praveen2112 on specific exception or all? Also there should be some recovery after some time?

huberty89 · 2023-07-28T14:30:41Z

@Praveen2112 @findepi @kokosing I moved config property, add a tests and in case of thrift transport error I do a fallback to old mechanism. Please review

kokosing · 2023-07-31T13:32:04Z

I don't like this test. You are testing the mock here.

How about using some TestHiveWithDisabledBatchFecthConnectorSmokeTest for this?

I don't like this test. You are testing the mock here.

I'm only provide fake ThriftMetastoreClient (I saw this pattern in few tests) to get the expected behavior so I can test (ThriftHiveMetastore) if new added config property works.

I like new name, I will apply it.

I could write test which runs real HMS and create more than 100 schemas/tables but this test would be slow + I have to run HMS in specific version which has a mentioned bug.

kokosing · 2023-08-02T12:29:56Z

Can we use io.trino.plugin.hive.metastore.thrift.ThriftHiveMetastoreClient#alternativeCall for that?

Please mention in the message that there will be a fallback to a different method to retrieve the information.

I guess HMS is not going to change so we should not repeat calling wrong API for the next request again.

~~Not sure if that easy, in that case I have to recreate and duplicate logic from:~~

io.trino.plugin.hive.HiveMetadata#listTables io.trino.plugin.hive.HiveMetadata#listViews io.trino.plugin.hive.HiveMetadata#listSchemaNames

Edit: hold on, I need to very this ^

I guess HMS is not going to change so we should not repeat calling wrong API for the next request again.

From https://issues.apache.org/jira/browse/HIVE-21028 it looks like particular problem was related to race condition, this is how author of the fix reproduce it locally:

I have manually tested this by slowing down the get_table_meta call and then dropping the database from another HMS.

So even with old version of HMS this should work in most of the cases.

Or should we retry on a specific exception type ?

The issue looks like a transient one so we could retry the next time

Or should we retry on a specific exception type ?

I think there is not specific exception, just network related error:

Caused by: org.apache.thrift.transport.TTransportException: ip-10-210-216-228.ap-northeast-2.compute.internal:9083: Socket is closed by peer.

@kokosing @Praveen2112 I try to implement this using alternativeCall. The implementation itself is trivial, but writing a tests for that is challenging. In that case I have to refactor ThriftHiveMetastoreClient so ThriftHiveMetastore.Iface(client) could be provided from outside but later creating test implementation of ThriftHiveMetastore.Iface with 211 methods is not easy to read.

heechan3006 · 2023-08-21T09:32:30Z

@huberty89 Is this ready for merging?

Praveen2112

Post applying @kokosing & @ksobolew 's pending comments

Praveen2112 · 2023-08-07T09:12:54Z

Or should we retry on a specific exception type ?

Praveen2112 · 2023-08-21T09:45:14Z

The issue looks like a transient one so we could retry the next time

kokosing

LGTM % remaining comments

For older versions of HMS fetching all tables and all views may end up in failure. https://issues.apache.org/jira/browse/HIVE-21028 In such case we need to use previous mechanism and fetch tables/views from each schema separately

kokosing · 2023-08-28T12:45:28Z

Thank you!

colebow · 2023-08-30T19:38:54Z

Added docs in #18870 - but in the future, please make sure new configuration properties are documented when you add them @huberty89

cla-bot bot added the cla-signed label Jul 13, 2023

huberty89 requested review from Praveen2112, ebyhr, findepi and kokosing July 13, 2023 10:26

huberty89 force-pushed the huberty89/batch-metadata-fetch-switch branch 2 times, most recently from 5ee36fd to ef8d13d Compare July 13, 2023 10:31

github-actions bot added tests:hive hive Hive connector labels Jul 13, 2023

huberty89 force-pushed the huberty89/batch-metadata-fetch-switch branch from ef8d13d to db3c754 Compare July 13, 2023 13:25

Praveen2112 reviewed Jul 13, 2023

View reviewed changes

Comment thread plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java Outdated

kokosing reviewed Jul 14, 2023

View reviewed changes

Comment thread plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java Outdated

huberty89 force-pushed the huberty89/batch-metadata-fetch-switch branch from db3c754 to 1884958 Compare July 27, 2023 10:32

huberty89 force-pushed the huberty89/batch-metadata-fetch-switch branch 2 times, most recently from ffc0105 to c9cd823 Compare July 28, 2023 14:12

findepi requested review from Praveen2112 and kokosing July 31, 2023 10:47

kokosing mentioned this pull request Jul 31, 2023

Fetching all hive metadata failed issues #18111

Closed

kokosing reviewed Jul 31, 2023

View reviewed changes

huberty89 force-pushed the huberty89/batch-metadata-fetch-switch branch from c9cd823 to 387c22a Compare August 2, 2023 10:47

huberty89 requested a review from kokosing August 2, 2023 10:49

kokosing reviewed Aug 2, 2023

View reviewed changes

ksobolew reviewed Aug 2, 2023

View reviewed changes

Comment thread .../io/trino/plugin/hive/metastore/thrift/TestHiveWithDisabledBatchFetchConnectorSmokeTest.java Outdated

Comment thread .../io/trino/plugin/hive/metastore/thrift/TestHiveWithDisabledBatchFetchConnectorSmokeTest.java Outdated

Praveen2112 approved these changes Aug 21, 2023

View reviewed changes

kokosing reviewed Aug 21, 2023

View reviewed changes

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

huberty89 force-pushed the huberty89/batch-metadata-fetch-switch branch from 387c22a to 8da5e17 Compare August 23, 2023 12:17

ksobolew reviewed Aug 23, 2023

View reviewed changes

Comment thread plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java Outdated

ksobolew reviewed Aug 23, 2023

View reviewed changes

Comment thread ...hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestHiveWithDisabledBatchFetch.java Outdated

Hubert Łojek added 2 commits August 23, 2023 19:36

Add ability to disable batch fetch tables and views from HMS

f5ff768

huberty89 force-pushed the huberty89/batch-metadata-fetch-switch branch from 8da5e17 to 4979b15 Compare August 23, 2023 17:45

ksobolew approved these changes Aug 24, 2023

View reviewed changes

kokosing merged commit 15c5b74 into trinodb:master Aug 28, 2023

github-actions bot added this to the 426 milestone Aug 28, 2023

This was referenced Aug 30, 2023

Add Trino 426 release notes #18867

Merged

Document batch fetch config property #18870

Merged

Conversation

huberty89 commented Jul 13, 2023 • edited by hashhar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

huberty89 commented Jul 13, 2023

Uh oh!

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huberty89 commented Jul 27, 2023

Uh oh!

Praveen2112 commented Jul 27, 2023

Uh oh!

huberty89 commented Jul 27, 2023

Uh oh!

huberty89 commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huberty89 Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huberty89 Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

heechan3006 commented Aug 21, 2023

Uh oh!

Praveen2112 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kokosing commented Aug 28, 2023

Uh oh!

colebow commented Aug 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

huberty89 commented Jul 13, 2023 •

edited by hashhar

Loading

huberty89 commented Jul 28, 2023 •

edited

Loading

huberty89 Jul 31, 2023 •

edited

Loading

huberty89 Aug 3, 2023 •

edited

Loading