Support extra http headers in pinot controller/broker requests

[xiangfu0]

Description

Allow users to add customized HTTP headers for controller/broker requests.
This is useful for users who require customization, tracing, or tag information.

Documentation

( ) No documentation is needed.
(X) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(X) Release notes entries required with the following suggested text:

# Pinot
* Support extra HTTP headers in pinot controller/broker requests.

[ebyhr]

I have little concern to have such property that we can't test easily. For instance, when users try to set this property, face some issue and ask in Slack or somewhere, it' basically hard to help them in my opinion. Could you share more details about "customization, tracing, or tag information" and describe why dedicate properties don't work?

[xiangfu0]

One use case here is that we want to add extra headers to let Pinot track the sources and tags of which presto cluster it queries.

Another use case is to allow the Pinot side to use different auth mechanisms, e.g. API key.

[ebyhr]

Thanks.

One use case here is that we want to add extra headers to let Pinot side track the sources and tags of which presto cluster it queries.

This doesn't explain why dedicate properties don't work. Do you mean Pinot doesn't have prepared configuration for sources and tags?

Another use case is to allow another auth mechanism in Pinot, e.g. using an API key.

We should add dedicated property when we want to support new auth mechanism.

[xiangfu0]

This is the catch all solution for all the other cases.

During dev/staging process, it may not worth to patch Trino, config changes will be much simpler.

[ebyhr]

If the purpose is for dev/staging process, the current property name is incorrect and we should hide from documentation in my opinion.

[xiangfu0]

No. It's not for dev purpose. I'm just listing all the scenarios this thing can be useful.

This is the catch all solution to allow users to customize what to pass to Pinot side.

From users perspective, those Pinot side customization may not be put into open source code. And there will always be delay from implementing features in Pinot then enable it in Trino.

[xiangfu0]

@ebyhr can we merge this?

[hashhar]

IMO there's not very strong reason to allow arbitrary HTTP headers. That would basically remove any motivation to add proper support for things like different auth mechanisms or transports etc.

If there are specific features that this PR unlocks then let's focus on implementing those instead. The number one example that comes to mind is #13410

[xiangfu0]

This is a gap filling solution.

A simple ask is my company asked all services to add a custom http header 'X-MY-COMPANY-SRC', there is no way than fork Trino source code and apply this feature.

Another issue is missing this kind of thing preventing users from evaluating Trino comparing to the other solution. I cannot tell them to take this PR and build your own distribution.

[martint]

A few questions:

• What happens when keys and values contain characters that are not valid characters in the HTTP message header syntax?
• What happens if the user provides headers that conflict with standard HTTP headers (e.g., Content-Type, Accept, etc)
• What's the backward compatibility story if the connector starts sending a header in a new version that the user had configured manually?

[xiangfu0]

1. Technically user can send any character;
2. Extra headers are set first, then standard HTTP headers. See method: doHttpActionWithHeadersJson();
3. We call it user-side backward incompatible. Typically new features added may require a flag to turn on, so users should get a client-side warning or just an exception up to the feature implementor.

[xiangfu0]

Any update on review for this ?

[mosabua]

👋 @xiangfu0 @martint - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

[github-actions]

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

[mosabua]

Given the concerns from the reviewers and the fact that the PR has been stale for such a long time, I am closing this PR. If desired please reach out on Slack so we can discuss before you invest more time in rebasing or any other development.