Add Alluxio Hive metastore #13986

haoyuan · 2020-01-19T04:52:43Z

Co-authored-by: David Zhu <[email protected]>
Co-authored-by: Zac Blanco <[email protected]>
Co-authored-by: Calvin Jia <[email protected]>
Co-authored-by: Bin Fan <[email protected]>

== RELEASE NOTES ==

Hive Changes
This change introduces the Alluxio metastore which connects to the [Alluxio catalog service](https://docs.alluxio.io/os/user/2.1/en/core-services/Catalog.html).

highker

seemed through; structure wise looks good to me. There will be nits to fix. As a first step, could you fix the compilation error?

highker · 2020-01-20T02:38:30Z

pom.xml

This looks like an unrelated change

linux-foundation-easycla · 2020-01-21T18:22:14Z

The committers are authorized under a signed CLA.

✅ Haoyuan Li (667075b68e64a34218dcb4ffd124dd90b409941a)

yuzhu · 2020-01-21T18:32:43Z

@highker I am working with @haoyuan on this PR.
Fixed the compilation and pom files. Could you please take a pass and give comments?

Thanks. Also, I guess I should go through the CLA process on EasyCLA?

highker

Yes, you need to sign the CLA
Could you squash all the commits into one?
Could you follow the dev guideline and clean up the coding style (https://github.com/prestodb/presto/wiki/Presto-Development-Guidelines#formatting)? I have put some comments inline. Hope that will help

highker · 2020-01-22T16:39:09Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

highker · 2020-01-22T16:39:24Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

requireNonNull

highker · 2020-01-22T16:40:29Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

nit: getSupportedColumnStatistics is not supported in AlluxioHiveMetastore.

highker · 2020-01-22T16:40:58Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

nit: put with the previous line

highker · 2020-01-22T16:41:44Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

nit

Table table = getTable(databaseName, tableName).orElseThrow(() -> new PrestoException( HIVE_METASTORE_ERROR, String.format("Could not retrieve table %s.%s", databaseName, tableName)));

highker · 2020-01-22T16:42:26Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

nit: one param per line; keep the first line empty

highker · 2020-01-22T16:42:31Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

highker · 2020-01-22T16:42:33Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

highker · 2020-01-22T16:42:38Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

highker · 2020-01-22T16:42:55Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

highker

@yuzhu, could you update the commit title/message to be

Add Alluxio Hive metastore

haoyuan · 2020-01-22T21:46:25Z

@yuzhu, could you update the commit title/message to be
Add Alluxio Hive metastore

Done.

highker

Did skimmed through all the code. Could you fix the nits in the rest of the patch base on the coding style guideline?

Also, it might worth adding a test for AlluxioHiveMetastore as well

highker · 2020-01-22T21:46:50Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

nit

this.client = requireNonNull(client, "client is null");

highker · 2020-01-22T21:47:58Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

We usually use ImmutableMap.of

highker · 2020-01-22T21:48:05Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

highker · 2020-01-22T21:48:56Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

put this with the previous line

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

highker · 2020-01-22T21:52:58Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/ProtoUtils.java

spell out db

highker · 2020-01-22T21:53:25Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/ProtoUtils.java

put an empty line after this.

highker · 2020-01-22T21:54:28Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/ProtoUtils.java

(f) -> field

highker · 2020-01-22T21:55:04Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/ProtoUtils.java

spell out sd

highker · 2020-01-22T21:55:10Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/ProtoUtils.java

this method is not used?

highker · 2020-01-22T22:09:52Z

Once comment is address, feel free to click the "re-request review" button so I will get notified.

yuzhu · 2020-01-23T23:02:28Z

@highker Addressed your comment with the exception of adding test for AlluxioHiveMetastore

Regarding the test, I am planning to extend AbstractTestHive and use the docker containers to test the metastore. Does that sound like a reasonable plan to you?

I will also squash the commits once you have taken a pass. I want to separate the commits so it is easier to review.

highker

LGTM; minor nits only.

Could you squash all the commits into one?

highker · 2020-01-24T01:27:45Z

...tastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioMetastoreModule.java

highker · 2020-01-24T01:27:49Z

...tastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioMetastoreModule.java

not used; so is the one in the constructor

highker · 2020-01-24T01:28:28Z

...tastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioMetastoreModule.java

merge with the pervious line

highker · 2020-01-24T01:29:15Z

...ve-metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioProtoUtils.java

replace with Guava's stuff.

highker · 2020-01-24T01:30:12Z

...ve-metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioProtoUtils.java

merge into oneline

highker · 2020-01-24T01:31:41Z

...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java

nit

return partitionInfos.stream(). ...

yuzhu · 2020-01-24T01:41:29Z

@highker thanks, i will fix those tonight. do you have any thoughts re:testing.

I am planning to extend AbstractTestHive and use the docker containers to test the metastore. Does that sound like a reasonable plan to you?

highker · 2020-01-24T01:43:17Z

@yuzhu, if the test for the metastore module is too complicated, don't worry about that. The config test is good enough. Docker sounds an overkill.

yuzhu · 2020-01-24T02:23:54Z

@highker comments addressed, commits squashed. Let me know if there is anything else.

Thanks!

highker

lgtm

highker

Could you fix the test failure?

This change introduces the Alluxio metastore which connects to the Alluxio catalog service. See: https://docs.alluxio.io/os/user/2.1/en/core-services/Catalog.html for more information on the catalog service. In this PR we introduce the bare minimum to configure and use the Alluxio catalog. Reads are the only supported operation at the moment. Co-authored-by: David Zhu <[email protected]> Co-authored-by: Zac Blanco <[email protected]>

yuzhu · 2020-01-24T05:32:25Z

@highker fixed. Thanks.

highker self-requested a review January 20, 2020 00:29

highker reviewed Jan 20, 2020

View reviewed changes

highker self-requested a review January 21, 2020 18:35

highker reviewed Jan 22, 2020

View reviewed changes

haoyuan mentioned this pull request Jan 22, 2020

Support PathFilter when fetching splits for Hudi tables #13818

Merged

yuzhu force-pushed the alluxio-metastore branch 2 times, most recently from 667075b to 41079e2 Compare January 22, 2020 19:02

highker self-requested a review January 22, 2020 21:21

highker reviewed Jan 22, 2020

View reviewed changes

haoyuan changed the title ~~Add Alluxio Hive metastore to enable Presto with Alluxio Structured Data Service~~ Add Alluxio Hive metastore Jan 22, 2020

yuzhu force-pushed the alluxio-metastore branch from 41079e2 to 40ab2c9 Compare January 22, 2020 21:50

highker reviewed Jan 22, 2020

View reviewed changes

haoyuan requested a review from highker January 23, 2020 23:21

highker reviewed Jan 24, 2020

View reviewed changes

yuzhu force-pushed the alluxio-metastore branch from 9c67d17 to a6743cb Compare January 24, 2020 02:15

haoyuan requested a review from highker January 24, 2020 02:23

highker approved these changes Jan 24, 2020

View reviewed changes

highker self-assigned this Jan 24, 2020

highker reviewed Jan 24, 2020

View reviewed changes

yuzhu force-pushed the alluxio-metastore branch from a6743cb to f6f08bb Compare January 24, 2020 03:16

yuzhu force-pushed the alluxio-metastore branch from f6f08bb to acb25d1 Compare January 24, 2020 03:45

highker merged commit 5106dc1 into prestodb:master Jan 24, 2020

caithagoras mentioned this pull request Feb 20, 2020

Add release notes for 0.232 #14130

Merged

8 tasks

Add Alluxio Hive metastore #13986

Add Alluxio Hive metastore #13986

Uh oh!

Conversation

haoyuan commented Jan 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highker left a comment

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!

linux-foundation-easycla bot commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuzhu commented Jan 21, 2020

Uh oh!

highker left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

haoyuan commented Jan 22, 2020

Uh oh!

highker left a comment

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!

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!

highker commented Jan 22, 2020

Uh oh!

yuzhu commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highker left a comment

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

haoyuan commented Jan 19, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jan 21, 2020 •

edited

Loading

yuzhu commented Jan 23, 2020 •

edited

Loading