Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HIVE-26484: Add proto3 hivemetastore.proto #3784

Merged
merged 2 commits into from
Nov 22, 2022

Conversation

Noremac201
Copy link
Contributor

This is a rebased version of #3534,

What changes were proposed in this pull request?

  • Added a proto3 syntax Hive Metastore proto file that was based off the existing hive_metastore.thrift file.
    • It generally tries to follow grpc best practices, so there are some slight variations on thrift, for example enums, response types of response specific empty protos, snake_case.
    • Guava is updated to version 22, which is required for proto3/grpc for compiling the protos.

As this is "new" API, the new grpc API doesn’t include the same method with different signature, i.e. get_databases(string db_id) is usurped by get_databases(GetDatabasesRequest request).

Why are the changes needed?

The proto file is necessary because in order to have native gRPC support in Hive Metastore, there needs to be gRPC equivalents of all the preexisting Thrift objects as well as method header definitions in gRPC for Thrift counterparts.

Does this PR introduce any user-facing change?

Not yet, while this will allow developers to use this proto to generate clients (i.e. in Impala) before full support for gRPC is added in Hive Metastore.

How was this patch tested?

No tests were added in this PR other than making sure that the proto file and pom.xml files compile without issues and checking the target/ directory for generated files.

@deniskuzZ deniskuzZ changed the title [HIVE-26484] Add proto3 hivemetastore.proto HIVE-26484: Add proto3 hivemetastore.proto Nov 18, 2022
Copy link
Member

@deniskuzZ deniskuzZ left a comment

Choose a reason for hiding this comment

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

LGTM +1

Copy link
Contributor

@cnauroth cnauroth left a comment

Choose a reason for hiding this comment

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

+1 (non-binding)

Thank you for this contribution, @Noremac201 and @rohansonecha !

Rohan Sonecha and others added 2 commits November 21, 2022 10:05
Change-Id: I38dd936e4ad710e05e95d78f05b5e07585cb1660
@deniskuzZ
Copy link
Member

rebased again

@sonarcloud
Copy link

sonarcloud bot commented Nov 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@deniskuzZ deniskuzZ merged commit 585eac4 into apache:master Nov 22, 2022
dengzhhu653 pushed a commit to dengzhhu653/hive that referenced this pull request Dec 15, 2022
…by Chris Nauroth, Denys Kuzmenko)

Co-authored-by: Rohan Sonecha
Closes apache#3784
DongWei-4 pushed a commit to DongWei-4/hive that referenced this pull request Dec 29, 2022
…by Chris Nauroth, Denys Kuzmenko)

Co-authored-by: Rohan Sonecha
Closes apache#3784

(cherry picked from commit 585eac4)
@Aggarwal-Raghav
Copy link
Contributor

@cnauroth @deniskuzZ, one question: Any specific reason why guava version was not upgraded in standalone-metastore/pom.xml and storage-api/pom.xml? It is still 19.0!

@deniskuzZ
Copy link
Member

deniskuzZ commented Mar 13, 2023

@cnauroth @deniskuzZ, one question: Any specific reason why guava version was not upgraded in standalone-metastore/pom.xml and storage-api/pom.xml? It is still 19.0!

PR mentions that proto3/grpc won't compile with guava < 22, however, looks like standalane-metastore's root pom overrides it with v19. Not sure how it compiled then in the first place, maybe a version upgrade wasn't required.

mvn dependency:tree | grep guava
[INFO] +- com.google.guava:guava:jar:19.0:compile

There was no further contribution since the initial proto file generation, we might consider reverting this PR.

@cnauroth
Copy link
Contributor

I'm guessing this was an oversight, not an intentional choice to leave Guava at 19.0. @Noremac201 , maybe you'd be interested in sending a follow-up PR to upgrade Guava to 22.0 in standalone-metastore/pom.xml and storage-api/pom.xml?

@Aggarwal-Raghav
Copy link
Contributor

@cnauroth, I have created a PR for this: #4143
Kindly review.

yeahyung pushed a commit to yeahyung/hive that referenced this pull request Jul 20, 2023
…by Chris Nauroth, Denys Kuzmenko)

Co-authored-by: Rohan Sonecha
Closes apache#3784
tarak271 pushed a commit to tarak271/hive-1 that referenced this pull request Dec 19, 2023
…by Chris Nauroth, Denys Kuzmenko)

Co-authored-by: Rohan Sonecha
Closes apache#3784
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants