Add support for pinot proxy

[xiangfu0]

Description

Pinot proxy is used as the global user-facing endpoint to handle all controller/broker HTTP requests as well as server gRPC requests for Trino. This is a must when connecting pinot inside a k8s cluster externally.

Adding support to enable pinot proxy for broker and grpc requests.
This feature is default disabled and can be enabled by specifying the rest proxy REST/GRPC endpoints:

pinot.controller-urls=https://my-pinot-rest-proxy:8123
pinot.proxy.enabled=true
pinot.grpc.proxy-uri=my-pinot-rest-proxy:8124

Release notes

# Pinot Changes
* Adding support to enable pinot proxy for controller/broker and server grpc requests.

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

[ebyhr]

Please fix the commit title to follow this guideline.
https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#commits-and-pull-requests

[xiangfu0]

Will rebase once: #13410 and #13469 are merged

[hashhar]

some editorial comments.

can you explain why multiple proxy configs need to exist? Also does the proxy remove the need to configure the controller url?
Or are they mutually exclusive? If yes then the config classes should assert that otherwise it's both hard for user to understand what to configure when and also for reviewers/future authors to understand that.

[hashhar]

Much simpler than before. Thanks.

Some nits but looks good otherwise.

[xiangfu0]

Thanks for the review!

[martint]

Given that the controller and the proxy are API compatible, I'm not sure I understand why we need a separate pinot.grpc.proxy-uri instead of just reusing the host/port for the controller.

[xiangfu0]

Given that the controller and the proxy are API compatible, I'm not sure I understand why we need a separate pinot.grpc.proxy-uri instead of just reusing the host/port for the controller.

The current connector requires REST API access to Pinot Controller/Broker, for cluster/table management and broker queries those APIs are made compatible in Pinot Proxy, so we can reuse proxy REST endpoint for controller, e.g. https://proxy.my-pinot-cluster:443

This connector also requires GRPC access to Pinot Servers for server queries data transfer, which are not exposed outside k8s. So we need to expose them through Pinot Proxy and build a Load Balancer on that, hence the new URI, e.g. grpc.proxy.my-pinot-cluster:443

[martint]

I'm still a little confused.

When pinot.proxy.enabled is true, what is the role of pinot.grpc.port compared to pinot.grpc.proxy-uri. Does pinot.grpc.port get used at all? What for?

[xiangfu0]

I'm still a little confused.

When pinot.proxy.enabled is true, what is the role of pinot.grpc.port compared to pinot.grpc.proxy-uri. Does pinot.grpc.port get used at all? What for?

So this pinot.grpc.port remains the same as the Pinot server grpc port.

pinot.grpc.proxy-uri is the entry point for external grpc access. Inside the proxy it will connect to the server grpc endpoint ( configured in pinot.grpc.port) for server queries.

[martint]

What's the purpose of extending from GrpcRequestBuilder? There's not an "is a" relationship between the two classes, and this class seems to be overriding every method from the parent that's used.

In fact, the only method that's used from the parent is a call to setBrokerRequest(), but the build() method does not use that value at all.

[xiangfu0]

Good point, Pinot 0.10+ has deprecated the BrokerRequest, so we actually don't have to keep it.
And the PinotProxyGrpcRequestBuilder doesn't need to extend GrpcRequestBuilder.

[xiangfu0]

Updated the PR and rebase with Pinot 0.11 upgrade

[martint]

Looks good. Can you squash the commits and change the text to "Add support for pinot proxy"?

[xiangfu0]

Looks good. Can you squash the commits and change the text to "Add support for pinot proxy"?

Thanks! Done!

[xiangfu0]

Saw a failing https://github.com/trinodb/trino/actions/runs/3212258524/jobs/5251078085
Seems the master branch is also failing. Will wait for the fix and rebase.

[xiangfu0]

Saw a failing https://github.com/trinodb/trino/actions/runs/3212258524/jobs/5251078085 Seems the master branch is also failing. Will wait for the fix and rebase.

Rebased and passed the CI.

[xiangfu0]

Many thanks @martint !