Skip to content

Support Pinot with secure connection#13410

Merged
martint merged 1 commit intotrinodb:masterfrom
xiangfu0:pinot-secure-connection
Aug 26, 2022
Merged

Support Pinot with secure connection#13410
martint merged 1 commit intotrinodb:masterfrom
xiangfu0:pinot-secure-connection

Conversation

@xiangfu0
Copy link
Copy Markdown
Contributor

@xiangfu0 xiangfu0 commented Jul 29, 2022 •
edited
Loading

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 requested review from ddcprg, ebyhr and elonazoulay July 29, 2022 09:11
@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
@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 5 times, most recently from b2c45e3 to d87bb25 Compare July 29, 2022 10:44
ebyhr
ebyhr reviewed Jul 29, 2022
View reviewed changes
@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 2 times, most recently from dfdb40d to ce2c169 Compare July 29, 2022 19:05
ebyhr
ebyhr reviewed Jul 29, 2022
View reviewed changes
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!

ebyhr
ebyhr reviewed Aug 5, 2022
View reviewed changes
@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch from ac51c34 to f6f0836 Compare August 5, 2022 11:18
@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 xiangfu0 force-pushed the pinot-secure-connection branch from 4f82b2a to 9f88fe8 Compare August 10, 2022 04:43
@xiangfu0
Copy link
Copy Markdown
Contributor Author

xiangfu0 commented Aug 10, 2022

@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
Copy link
Copy Markdown
Member

@martint martint Aug 20, 2022

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
martint
martint reviewed Aug 26, 2022
View reviewed changes
@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
@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch 2 times, most recently from 304b0ad to 88ee8d4 Compare August 26, 2022 20:13
@xiangfu0 xiangfu0 force-pushed the pinot-secure-connection branch from 88ee8d4 to 18e7867 Compare August 26, 2022 20:45
@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
Copy Markdown
Contributor Author

xiangfu0 commented Aug 26, 2022

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

@ebyhr ebyhr Awaiting requested review from ebyhr

+2 more reviewers

@ddcprg ddcprg ddcprg left review comments

@elonazoulay elonazoulay elonazoulay approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed docs

Milestone

394

Development

Successfully merging this pull request may close these issues.

5 participants

@xiangfu0 @martint @elonazoulay @ebyhr @ddcprg