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.

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

Upgrade okhttp to 4.10.0 #16774

Merged
merged 3 commits into from
Apr 5, 2023
Merged

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Mar 29, 2023

Release notes are not needed.

@cla-bot cla-bot bot added the cla-signed label Mar 29, 2023
@wendigo wendigo requested review from electrum, findepi and dain and removed request for electrum and findepi March 29, 2023 08:41
@wendigo wendigo requested a review from findepi March 29, 2023 08:42
@wendigo wendigo requested review from hashhar and kokosing and removed request for dain March 29, 2023 08:42
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

The update itself looks good to me however I'll defer the merge to @electrum on this.

@wendigo
Copy link
Contributor Author

wendigo commented Mar 29, 2023

CI hit: #16653

@sajjoseph
Copy link
Contributor

Thanks for this change. I have been using OkHttp 4.10.0 in production with Trino for a while.
You could address the TODO entry in JsonResponse.java with OkHttp upgrade.

// TODO: fix in OkHttp: https://github.com/square/okhttp/issues/3111

@wendigo
Copy link
Contributor Author

wendigo commented Mar 30, 2023

@sajjoseph does it work? :)

@sajjoseph
Copy link
Contributor

@sajjoseph does it work? :)
It does!

Matter of fact I added HTTP/2 support in our version of Trino (I am contributing that change to airlift shortly) and OkHttp module automatically switched to HTTP/2 to communicate with Coordinator. I haven't seen any issues in our test environment.

We will be promoting it to prod in a week and so we will see how older and newer clients (JDBC, CLI etc with different OkHttp versions) are behaving.

@wendigo
Copy link
Contributor Author

wendigo commented Mar 30, 2023

That's great news @sajjoseph

@wendigo wendigo force-pushed the serafin/upgrade-okhttp-v2 branch from 1b9daff to 8c00081 Compare March 30, 2023 13:51
@wendigo
Copy link
Contributor Author

wendigo commented Mar 31, 2023

Ping @electrum

@wendigo wendigo force-pushed the serafin/upgrade-okhttp-v2 branch from 8c00081 to f9fa9f7 Compare April 3, 2023 08:56
@wendigo wendigo requested review from hashhar and kokosing April 3, 2023 08:56
wendigo added 2 commits April 3, 2023 14:30
They were removed in OkHttp 4.x and we still rely on the legacy SSL hostname verification
@wendigo wendigo force-pushed the serafin/upgrade-okhttp-v2 branch from f9fa9f7 to 1369b84 Compare April 3, 2023 12:31
@kokosing kokosing merged commit 4be1401 into trinodb:master Apr 5, 2023
@kokosing
Copy link
Member

kokosing commented Apr 5, 2023

Merged, thanks!

@github-actions github-actions bot added this to the 412 milestone Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants