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.

Sign up for GitHub

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

Jump to bottom

Pinot driver change to support full TLS in gRPC #17525

Merged
merged 1 commit into from
Apr 4, 2022
Merged

Pinot driver change to support full TLS in gRPC #17525

merged 1 commit into from
Apr 4, 2022

Conversation

dongxiaoman
Copy link
Contributor

@dongxiaoman dongxiaoman commented Mar 24, 2022 •
edited by highker
Loading

Test plan

Locally unit tested some code;
Locally connects to a Production Pinot cluster via secure TLS feature, ran the select * from mytable command to fully retrieve more than 8 rows successfully.

== RELEASE NOTES ==

Pinot Changes
* Upgrade Pinot connector to support Pinot 0.10.0
* Support TLS in Pinot gRPC connection

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 24, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: dongxiaoman / name: Xiaoman Dong (57f06e0e1b77c89177eead741497f648c8eac490)

@dongxiaoman dongxiaoman changed the title [wip] Pinot driver change to support full TLS in gRPC Pinot driver change to support full TLS in gRPC Mar 29, 2022
xiangfu0
xiangfu0 reviewed Mar 29, 2022
View reviewed changes
pom.xml Outdated
@@ -1724,14 +1724,26 @@
<groupId>org.apache.pinot</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

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

can you check if you can remove the shaded classifier on line 1721 and keep those pinot-*-jdk8 dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some dependency failure by enforcer. Will try more and ping you later
(Some sample error:

Require upper bound dependencies error for org.checkerframework:checker-qual:2.5.2 paths to dependency are:
+-com.facebook.presto:presto-pinot-toolkit:0.273-SNAPSHOT
  +-com.google.guava:guava:26.0-jre
    +-org.checkerframework:checker-qual:2.5.2
and
+-com.facebook.presto:presto-pinot-toolkit:0.273-SNAPSHOT
  +-org.apache.pinot:presto-pinot-driver:0.10.0
    +-org.apache.pinot:pinot-common-jdk8:0.10.0
      +-org.apache.calcite:calcite-core:1.29.0
        +-org.checkerframework:checker-qual:3.10.0

dongxiaoman
dongxiaoman commented Mar 29, 2022
View reviewed changes
presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotConfig.java
}

@Config("pinot.rest-proxy-url")
public PinotConfig setRestProxyUrl(String restProxyUrl)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nausicaasnow Can you help review whether this property is used by your side? We are trying to reduce the config values to avoid confusion, and the pinot.rest-proxy-url value is merged to be the controllerURL (hide the rest-proxy concept)

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @dharakk @agrawaldevesh This requires pinot proxy to implement the same API for controller(fetching table configs/schemas/etc) and broker to fetch routingTable, etc.

@dongxiaoman dongxiaoman force-pushed the xd-presto-pinot-v2 branch 3 times, most recently from 310abbd to 54c0abd Compare March 31, 2022 15:37
@xiangfu0 xiangfu0 force-pushed the xd-presto-pinot-v2 branch from dbec5c7 to f9ab484 Compare April 2, 2022 08:08
@xiangfu0
Copy link
Contributor

xiangfu0 commented Apr 2, 2022

@highker please take a look ! Thanks !

xiangfu0
xiangfu0 approved these changes Apr 2, 2022
View reviewed changes
Copy link
Contributor

@xiangfu0 xiangfu0 left a comment

Choose a reason for hiding this comment

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

lgtm!

@highker highker self-requested a review April 2, 2022 17:00
@highker
Copy link
Contributor

highker commented Apr 3, 2022

High-level comments: can we squash commits into logical units? There are so many cleanup-ish commits that do not fit. I don't mind having all squashing into one. If there is better structure, it would even better.

highker
highker approved these changes Apr 3, 2022
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

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

I will say just squash all commits into one with a good commit title and body

@dongxiaoman dongxiaoman requested a review from highker April 4, 2022 16:51
@dongxiaoman
Copy link
Contributor Author

I will say just squash all commits into one with a good commit title and body

@highker Thanks a lot for reviewing!. The commits are squashed. Please have another look

@highker highker merged commit 27579d8 into prestodb:master Apr 4, 2022
@mshang816 mshang816 mentioned this pull request May 17, 2022
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiangfu0 xiangfu0 xiangfu0 approved these changes

@highker highker Awaiting requested review from highker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dongxiaoman @xiangfu0 @highker