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

Support Pinot with secure connection #13410

Merged
merged 1 commit into from Aug 26, 2022
Merged

Support Pinot with secure connection #13410

merged 1 commit into from Aug 26, 2022

Conversation

xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Jul 29, 2022 •
edited

Description

Support pinot TLS connection to controller/broker when the pinot.controller-urls configuration uses https as the scheme.

Release notes

# Pinot
Support TLS connection when the `pinot.controller-urls` configuration uses `https` as the scheme.

@cla-bot cla-bot bot added the cla-signed label Jul 29, 2022
@github-actions github-actions bot added the docs label Jul 29, 2022
@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 3 times, most recently from d09abbc to 17f0dd1 Compare July 29, 2022 09:21
ebyhr
ebyhr reviewed Jul 29, 2022
View reviewed changes
docs/src/main/sphinx/connector/pinot.rst Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java Outdated Show resolved Hide resolved
@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 5 times, most recently from b2c45e3 to d87bb25 Compare July 29, 2022 10:44
@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 2 times, most recently from dfdb40d to ce2c169 Compare July 29, 2022 19:05
@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 2 times, most recently from a3b2caf to dbbb139 Compare July 30, 2022 07:35
ddcprg
ddcprg reviewed Jul 31, 2022
View reviewed changes
Copy link
Member

@ddcprg ddcprg left a comment

Choose a reason for hiding this comment

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

overall looks good to me with the requested changes, I'll let @ebyhr approve

@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 3 times, most recently from a9f131b to ac51c34 Compare August 1, 2022 23:38
@xiangfu0 xiangfu0 requested a review from ebyhr August 2, 2022 07:37
@xiangfu0
Copy link
Contributor Author

xiangfu0 commented Aug 3, 2022

@ebyhr can you take another pass for this

@xiangfu0 xiangfu0 mentioned this pull request Aug 4, 2022
Merged
elonazoulay
elonazoulay approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@elonazoulay elonazoulay left a comment

Choose a reason for hiding this comment

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

Overall lgtm!

@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 7 times, most recently from 8f2a246 to 4f82b2a Compare August 9, 2022 21:33
@xiangfu0 xiangfu0 requested a review from ebyhr August 9, 2022 21:34
@xiangfu0
Copy link
Contributor Author

@ebyhr can you take another pass?

@xiangfu0 xiangfu0 requested a review from martint August 18, 2022 22:31
martint
martint reviewed Aug 20, 2022
View reviewed changes
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java Outdated
@@ -258,4 +272,38 @@ public void validate()
!countDistinctPushdownEnabled || aggregationPushdownEnabled,
"Invalid configuration: pinot.aggregation-pushdown.enabled must be enabled if pinot.count-distinct-pushdown.enabled");
}

@AssertTrue(message = "Controller URL should use HTTPS scheme when TLS is enabled")
Copy link
Member

Choose a reason for hiding this comment

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

Why not just decide whether to use TLS or not based on the url scheme. It seems better than having two flags that need to agree.

@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 2 times, most recently from 2e3bb18 to 944a60a Compare August 26, 2022 00:21
@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 4 times, most recently from ec3f195 to 987288d Compare August 26, 2022 09:31
martint
martint reviewed Aug 26, 2022
View reviewed changes
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java Outdated Show resolved Hide resolved
@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 2 times, most recently from 304b0ad to 88ee8d4 Compare August 26, 2022 20:13
@martint martint merged commit 56c97c5 into trinodb:master Aug 26, 2022
@xiangfu0 xiangfu0 deleted the pinot-secure-connection branch August 26, 2022 21:32
@xiangfu0
Copy link
Contributor Author

Thanks @martint !

@github-actions github-actions bot added this to the 394 milestone Aug 26, 2022
@colebow colebow mentioned this pull request Aug 29, 2022
Merged
@ebyhr ebyhr mentioned this pull request Jun 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martint martint martint left review comments

@ddcprg ddcprg ddcprg left review comments

@elonazoulay elonazoulay elonazoulay approved these changes

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees
No one assigned
Labels
cla-signed docs
Milestone
394
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@xiangfu0 @martint @elonazoulay @ebyhr @ddcprg