Modularize Hive Metastore by islamismailov · Pull Request #13601 · prestodb/presto

islamismailov · 2019-10-24T23:31:16Z

== RELEASE NOTES ==

Hive Changes
* Metastore interface is separated into a separate module to reduce monolithicness

highker

I feel the core part is to move out ExtendedHiveMetastore together with its implementation hopefully. Also, SemiTransactionalHiveMetastore might also need to be moved out as a stretch goal.

islamismailov · 2019-10-25T16:29:33Z

I can move ExtendedHiveMetastore into the new module too, and possibly the SemiTransactionalHiveMetastore too. I am afraid that might be too coupled and monolithic (e.g. classes like HiveType etc. I will try to do in separate commits so we can reason about it in pieces, as it might be easier.

islamismailov · 2019-10-25T18:47:53Z

Here are the three commits:

Modularize only necessary components
Add ExtendedHiveMetastore pieces to the module
Add SemiTransactionalHiveMetastore pieces to the module

highker

This is a good start! Let's discuss in person what component should go to the new module. The new module needs to be self contained and flexible enough to be re-used by many other connectors.

highker · 2019-10-28T02:54:56Z

pom.xml

How about calling it presto-hive-metastore?

highker

Two quick comments while I'm doing the review:

The test failure is related. It is reproducible locally.
Could you call the new module presto-hive-metastore?

islamismailov · 2019-11-01T17:35:25Z

This should be ready for the review now :)

highker

There are lots of code movement within a file. Can we keep them at the same place as they were?

highker · 2019-11-02T02:07:25Z

pom.xml

This doesn't seem to be necessary

highker · 2019-11-02T02:10:05Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/HdfsEnvironment.java

Can we keep this as it was?

highker · 2019-11-02T02:38:21Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/HdfsEnvironment.java

just call this config

highker · 2019-11-02T02:38:29Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/HiveBasicStatistics.java

highker · 2019-11-02T02:39:02Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/HiveColumnHandle.java

Keep this file as is

highker

Minor comments; otherwise looks good to me

highker · 2019-11-05T21:36:53Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/MetastoreErrorCode.java

Shall we make 0x0100_0000 a constant and use it in both MetastoreErrorCode and HiveErrorCode?

highker · 2019-11-05T21:38:09Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/MetastoreErrorCode.java

Let's complete the comment to say "Shared error code with HiveErrorCode".

highker · 2019-11-05T21:39:17Z

presto-hive/pom.xml

accident change?

highker · 2019-11-05T21:41:32Z

presto-hive/src/test/java/com/facebook/presto/hive/AbstractTestHiveFileSystem.java

unrelated change?

highker · 2019-11-05T21:41:51Z

presto-hive/src/test/java/com/facebook/presto/hive/HiveQueryRunner.java

unrelated change?

highker · 2019-11-05T21:41:54Z

presto-hive/src/test/java/com/facebook/presto/hive/HiveQueryRunner.java

unrelated change?

highker · 2019-11-05T21:42:36Z

presto-hive/src/test/java/com/facebook/presto/hive/HiveTestUtils.java

Let's make sure this file has no unrelated code movement

highker · 2019-11-05T21:43:00Z

presto-hive/src/test/java/com/facebook/presto/hive/metastore/TestHiveMetastoreUtil.java

highker · 2019-11-05T21:43:09Z

...ive/src/test/java/com/facebook/presto/hive/metastore/thrift/TestThriftHiveMetastoreUtil.java

highker · 2019-11-05T22:54:34Z

@islamismailov, could you address the comments?

islamismailov · 2019-11-06T01:11:14Z

oh whoops! I thought i did push - sorry i forgot to press enter!

highker

Some more comments

Can we keep HiveColumnHandle and TestHiveColumnHandle inside presto-hive. HiveColumnHandle is part of the Hive connector, it shouldn't be separated out. To make this work, just move the only caller validateColumns as a private method to HiveMetadata
Same for HivePartitionKey. It is part of the HiveSplit and then part of hive connector. There is a place HiveTypeName gets it referenced. The INSTANCE_SIZE is wrong. Could you fix that?

highker · 2019-11-06T07:16:56Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveUtil.java

highker · 2019-11-06T07:18:14Z

presto-hive/src/test/java/com/facebook/presto/hive/AbstractTestHiveFileSystem.java

keep this line

highker · 2019-11-06T07:19:08Z

presto-hive/src/test/java/com/facebook/presto/hive/HiveTestUtils.java

Do not move this around

highker · 2019-11-06T07:19:59Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHivePartitionManager.java

keep this line

highker · 2019-11-06T07:20:06Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHivePartitionManager.java

highker

LGTM! @jessesleeping, do you have extra comments?

linux-foundation-easycla · 2019-11-08T03:10:51Z

The committers are authorized under a signed CLA.

✅ Islam Ismailov (9d6836ecb82634f9e95908547ffc9ddf4ad4f92c)

islamismailov · 2019-11-08T18:00:05Z

rebasing...

highker

one last comment

highker · 2019-11-08T18:40:52Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/HdfsEnvironment.java

keep this line

… module

facebook-github-bot added the CLA Signed label Oct 24, 2019

islamismailov force-pushed the modularize-hive-metastore branch 2 times, most recently from 8331662 to e8f4301 Compare October 25, 2019 04:11

highker self-requested a review October 25, 2019 05:51

highker reviewed Oct 25, 2019

View reviewed changes

islamismailov force-pushed the modularize-hive-metastore branch from 4baee89 to 4f3fc99 Compare October 25, 2019 18:46

islamismailov requested a review from highker October 25, 2019 19:21

highker reviewed Oct 28, 2019

View reviewed changes

pom.xml Outdated

Copy link

highker Oct 28, 2019

Choose a reason for hiding this comment

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

How about calling it presto-hive-metastore?

islamismailov force-pushed the modularize-hive-metastore branch 3 times, most recently from 1ce6cb5 to a9f55aa Compare October 31, 2019 20:51

islamismailov requested a review from highker October 31, 2019 23:39

highker reviewed Nov 1, 2019

View reviewed changes

islamismailov force-pushed the modularize-hive-metastore branch 2 times, most recently from 5c082ca to 09a7c09 Compare November 1, 2019 07:07

highker self-requested a review November 1, 2019 07:13

islamismailov force-pushed the modularize-hive-metastore branch from 09a7c09 to af8ce24 Compare November 1, 2019 17:34

islamismailov force-pushed the modularize-hive-metastore branch from af8ce24 to 1ebdb54 Compare November 1, 2019 18:14

highker reviewed Nov 2, 2019

View reviewed changes

islamismailov force-pushed the modularize-hive-metastore branch 2 times, most recently from 79a8f90 to 24602fa Compare November 4, 2019 20:32

islamismailov requested a review from highker November 4, 2019 20:33

islamismailov force-pushed the modularize-hive-metastore branch from 24602fa to f46a8aa Compare November 5, 2019 18:15

highker requested a review from jessesleeping November 5, 2019 21:34

highker reviewed Nov 5, 2019

View reviewed changes

islamismailov requested a review from highker November 5, 2019 22:50

highker removed their request for review November 5, 2019 22:54

islamismailov force-pushed the modularize-hive-metastore branch from f46a8aa to 0a7ac43 Compare November 6, 2019 01:11

highker self-requested a review November 6, 2019 01:13

highker reviewed Nov 6, 2019

View reviewed changes

islamismailov force-pushed the modularize-hive-metastore branch from 0a7ac43 to 56e1d3e Compare November 6, 2019 17:24

islamismailov requested a review from highker November 6, 2019 17:25

highker approved these changes Nov 6, 2019

View reviewed changes

highker self-assigned this Nov 6, 2019

islamismailov force-pushed the modularize-hive-metastore branch from 56e1d3e to 9d6836e Compare November 8, 2019 03:10

islamismailov force-pushed the modularize-hive-metastore branch from 9d6836e to 8b8cc24 Compare November 8, 2019 17:57

islamismailov force-pushed the modularize-hive-metastore branch from 8b8cc24 to 3da78f6 Compare November 8, 2019 18:11

highker reviewed Nov 8, 2019

View reviewed changes

presto-hive-metastore/src/main/java/com/facebook/presto/hive/HdfsEnvironment.java Outdated

Copy link

highker Nov 8, 2019

Choose a reason for hiding this comment

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

keep this line

Move ExtendedHiveMetastore with its deps to new presto-hive-metastore…

eb341d2

… module

islamismailov force-pushed the modularize-hive-metastore branch from 3da78f6 to eb341d2 Compare November 8, 2019 19:01

islamismailov requested a review from highker November 8, 2019 20:28

highker merged commit 669f9b5 into prestodb:master Nov 8, 2019

leiqingc mentioned this pull request Dec 10, 2019

[DO NOT MERGE] Add release notes for 0.230 #13836

Closed

9 tasks

This was referenced Dec 10, 2019

[DO NOT MERGE] Add release notes for 0.230 #13837

Closed

[DO NOT MERGE] Add release notes for 0.230 #13840

Closed

caithagoras mentioned this pull request Dec 18, 2019

Add release notes for 0.230 #13876

Closed

14 tasks

This was referenced Jan 2, 2020

Add release notes for 0.230 #13913

Closed

Add release notes for 0.230 #13915

Merged

Conversation

islamismailov commented Oct 24, 2019

Uh oh!

highker left a comment • 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!

islamismailov commented Oct 25, 2019

Uh oh!

islamismailov commented Oct 25, 2019

Uh oh!

highker left a comment

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!

islamismailov commented Nov 1, 2019

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!

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!

highker commented Nov 5, 2019

Uh oh!

islamismailov commented Nov 6, 2019

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!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

islamismailov commented Nov 8, 2019

Uh oh!

highker left a comment •

edited

Loading

linux-foundation-easycla bot commented Nov 8, 2019 •

edited

Loading