-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[native] Add header "Host" to PrestoExchangeSource #25272
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.
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
Conversation
|
@amitkdutta can you please help review? Thanks. |
|
@majetideepak : Is it possible to add a check in some test in https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/tests/PrestoExchangeSourceTest.cpp, so that we can be assured that this field continues to output in header and is not accidentally dropped by someone else. Else this PR is good, I'll approve it once the test is in place. |
|
@aditi-pandit We need to setup https with certain versions of packages. We don't have such a framework. |
|
I also don't see a way to get the |
|
You should be able to get host header from message parameter here (its unused now): https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/tests/PrestoExchangeSourceTest.cpp#L135 We can add a check that it always has a host:port format. |
ffea449 to
da31121
Compare
Co-authored-by: Yabin Ma <[email protected]>
da31121 to
c252a03
Compare
|
@aditi-pandit can you take a look now? I added a test. Thanks. |
aditi-pandit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @majetideepak
|
@majetideepak I can test and let you know today if any Meta specific issue show up. |
|
@amitkdutta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
amitkdutta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in Meta specific environment. No issues observed. Thanks @majetideepak
Description
HTTP 1.1 requires
Hostheader. We are seeing a 400 Bad Request response otherwise in our setup.Release Notes
Please follow release notes guidelines and fill in the release notes below.