Add Connection Params for JDBC HTTP Protocol by adonley · Pull Request #15955 · prestodb/presto

adonley · 2021-04-17T05:06:45Z

Fixes an issue for Java > 8_242 where the default is HTTP/2 for users with a loadbalancer infront that accepts the protocol.

== RELEASE NOTES ==

General Changes
* Added connection param "protocols" that allows a user to specify which HTTP protocols the Presto client is allowed to use.

linux-foundation-easycla · 2021-04-17T05:06:48Z

The committers are authorized under a signed CLA.

✅ Andrew Donley (4751f80)

amarkowitz · 2021-05-13T23:57:35Z

@highker / @tdcmeehan - hello!

This change is more or less a patch pulled over from trinodb/trino#5915 which enables the prestodb driver to have a flag that will disable http2 when active. We have some customers who are hanging when the driver attempts to use http2 to a proxy that accepts http2, but the backend doesn't. This patch enables us to disable http2 for those scenarios.

We are hoping you might be able to guide our team here on the best path forward for any recommended cleanup in this PR and/or provide guidance on how we might get this patch merged.

Any help is much appreciated! Thanks!

amarkowitz · 2021-05-24T23:05:11Z

@electrum - I apologize for pulling you in to this thread, but we haven't yet had any guidance and aren't quite sure who else to reach out to. Our team here is hoping you might be able to help us on the best path forward for any recommended cleanup in this PR and/or provide guidance on how we might get this patch merged. This is solving a similar problem wrt jdk8u242 as trinodb/trino#5915. Any help is much appreciated! 🙏

tdcmeehan · 2021-05-24T23:14:26Z

Hello @amarkowitz and @adonley!

Firstly, looks like I missed your ping 11 days ago. I'll give this a review now.

In the future, feel free to ask on the PrestoDB Slack (https://prestodb.io/community.html) on the #dev channel to have eyes on reviews when needed.

tdcmeehan · 2021-05-24T23:15:24Z

presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoDriverUri.java

            setupHttpProxy(builder, HTTP_PROXY.getValue(properties));

+            if (isHttp2Disabled()) {
+                builder.protocols(Collections.singletonList(Protocol.HTTP_1_1));


Suggested change

builder.protocols(Collections.singletonList(Protocol.HTTP_1_1));

builder.protocols(ImmutableList.of(Protocol.HTTP_1_1));

Touched this up in the latest commit!

tdcmeehan · 2021-05-24T23:19:15Z

presto-jdbc/src/test/java/com/facebook/presto/jdbc/TestPrestoDriverUri.java

+    public void testUriWithoutHttp2()
+            throws SQLException
+    {
+        PrestoDriverUri parameters = createDriverUri("presto://localhost:8080/blackhole?disableHttp2=true");


Curious who uses this HTTP flag? Does this have some sort of significance to the HTTP client?

We have a few customers who are behind an LB that accepts HTTP2 which okhttp3 defaults to in recent Java 8 and 11 versions. The DB doesn't accept HTTP2 when it's forwarded along - these are the folks that use the flag. The flag itself disables HTTP2 and forces HTTP1.1 .

I was thinking, let's make it a flag just in-case someone has an existing config that needs HTTP2 and have the option to disable it for folks who need it in their setups.

amarkowitz · 2021-05-24T23:54:14Z

@tdcmeehan - No worries and thanks for the ref on how we should ping for future PRs 😸 @adonley is going to take it from here wrt reviews and CI fixups. Much appreciated 💯

pettyjamesm

You probably also want to make the configuration parameter usable via presto CLI ClientOptions (which will require changes to ClientSession and QueryRunner) as well as via BenchmarkDriverOptions. Take a look at #15393 for an example of a recent PR that added a connection property and the plumbing to those other code paths.

pettyjamesm · 2021-05-25T16:52:10Z

presto-jdbc/src/main/java/com/facebook/presto/jdbc/ConnectionProperties.java

+    {
+        public DisableHttp2()
+        {
+            super("disableHttp2", NOT_REQUIRED, ALLOWED, BOOLEAN_CONVERTER);


Instead of "disableHttp2", it might be better to use something like protocols={comma separated list} since OkHttp supports HTTP 1.0 separate from HTTP 1.1 as well as SPDY and proabably HTTP 3 eventually. The name of the parameter and the effect it actually has (forces HTTP 1.1) are misaligned.

Touched this up to add a list of protocols rather than a flag for HTTP 1.1. Made some easier mappings than what's provided by the OkHTTP enum and fallback to the enum in the case where we can't parse it.

I think the changes to the CLI / bench mark should come in a subsequent PR? Correct me if I'm wrong here.

Thanks for making the changes! I like the approach and the ability to accept the concise protocol names. In preparation to get these changes merge ready, check out the wiki section on commit formatting and pull requests, in particular- we want the commits that we merge to be a consumable changelog, so that means rebasing commits instead of merging from master as well as squashing commits into their logical atomic changes. In this context, I think the commits currently present could be reasonably squashed into a single commit (be sure to check out the linked commit message guidelines too!)

Regarding whether to add the CLI / Benchmark options in a separate PR, it depends. If you're willing to do the work to add those changes, it would be easier to review and merge them as a single PR with those additional changes in their own commits. If you don't have the time to make the extra changes required to do that, it's totally understandable and in that case we can talk about just merging the change to the JDBC driver for now and see if someone else can pick up the task of adding support to the CLI and Benchmark options later.

No problem, thank you for reviewing them! I just rebased and force pushed squashing all the changes.

I don't know that I'll have time right now to address CLI + Benchmark changes but I can consider making them at a later date.

pettyjamesm

I suggest adding a test case to TestJdbcConnection similar to TestJdbcConnection#testApplicationName where you pass HTTP 1.1 explicitly and verify that the connection is actually usable when the protocol flags are passed to the OkHttpClient.Builder.

pettyjamesm · 2021-05-28T14:59:13Z

presto-jdbc/src/main/java/com/facebook/presto/jdbc/AbstractConnectionProperty.java

+                        return Protocol.get(protocolName);
+                }
+            }
+            catch (Throwable e) {


This should be catch (Exception e) instead of Throwable. The problem is that subtypes of Error are also subtypes of Throwable but should be fatal and should not be caught. For instance, a broken OkHttp3 dependency version could throw UnsatisfiedLinkError and we wouldn't want to convert that into an IllegalArgumentException.

Made this Exception in latest push.

pettyjamesm · 2021-05-28T15:01:18Z

presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoDriverUri.java

        return DISABLE_COMPRESSION.getValue(properties).orElse(false);
    }

+    public List<Protocol> getProtocols()


I suggest returning Optional<List<Protocol>> here (where Optional.empty() is returned instead of an empty list) so that someone doesn't accidentally fail to check for the empty condition in other usage sites.

Made this Optional in latest push.

pettyjamesm · 2021-05-28T15:09:28Z

presto-jdbc/src/main/java/com/facebook/presto/jdbc/AbstractConnectionProperty.java

+                    case "http2":
+                        return Protocol.HTTP_2;
+                    case "spdy":
+                        return Protocol.SPDY_3;


I just looked at OkHttpClient.Builder#protocols and noticed this line:

... // Remove protocols that we no longer support. protocols.remove(Protocol.SPDY_3); ...

Based on that, I don't think it's worthwhile to make it impossible to pass- but do suggest that we don't give SPDY_3 a "friendly name".

Removed the friendly "spdy" option for protocols in latest push.

pettyjamesm · 2021-05-28T15:24:01Z

@tdcmeehan I'm ok with this PR after my last round of comments are addressed, do you want to weigh in on whether we want to merge this PR if it only supports passing the protocols list to JDBC drivers and not the CLI / Benchmark Runner? (context in this comment)

adonley · 2021-05-29T04:18:13Z

I suggest adding a test case to TestJdbcConnection similar to TestJdbcConnection#testApplicationName where you pass HTTP 1.1 explicitly and verify that the connection is actually usable when the protocol flags are passed to the OkHttpClient.Builder.

I added some tests for the protocols connection param. The server in the testing framework only responds to HTTP1.1 which makes sense. This protocol param was created for users that have an LB infront of their DB servers.

tdcmeehan · 2021-05-31T18:52:52Z

LGTM, but please update the documentation as well: https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/installation/jdbc.rst

adonley · 2021-06-01T17:13:16Z

LGTM, but please update the documentation as well: https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/installation/jdbc.rst

Added params to JDBC documentation.

adonley force-pushed the adonley/disable-http2 branch from 0e9c362 to 4751f80 Compare April 17, 2021 19:19

tdcmeehan self-assigned this May 24, 2021

tdcmeehan requested review from pettyjamesm and tdcmeehan May 24, 2021 23:14

tdcmeehan reviewed May 24, 2021

View reviewed changes

pettyjamesm requested changes May 25, 2021

View reviewed changes

adonley force-pushed the adonley/disable-http2 branch from 8bd5681 to 82c86f5 Compare May 27, 2021 20:16

adonley changed the title ~~Add connection param for disabling http2~~ Add Connection Params for JDBC HTTP Protocol May 27, 2021

pettyjamesm reviewed May 28, 2021

View reviewed changes

adonley force-pushed the adonley/disable-http2 branch from 82c86f5 to 6bcf7b2 Compare May 29, 2021 04:14

pettyjamesm approved these changes May 29, 2021

View reviewed changes

tdcmeehan approved these changes May 30, 2021

View reviewed changes

Add Connection Params for JDBC HTTP Protocol

0e1285b

adonley force-pushed the adonley/disable-http2 branch from 6bcf7b2 to 0e1285b Compare June 1, 2021 17:10

tdcmeehan merged commit add05d0 into prestodb:master Jun 2, 2021

jainxrohit mentioned this pull request Jun 5, 2021

Add release notes for 0.255 #16216

Merged

4 tasks

ajaygeorge mentioned this pull request Jun 9, 2021

Add release notes for 0.255 #16232

Closed

4 tasks

pettyjamesm mentioned this pull request Jun 9, 2021

Make HTTP protocol a configurable option #16235

Closed

rubenssoto mentioned this pull request Aug 23, 2021

PrestoDB JDBC driver doesn't handle protocols in Connection String metabase/metabase#17543

Closed

brokenjacobs mentioned this pull request Jan 11, 2022

JDK8 regression similar to #1169 with http2 enabled proxy. Slow exit trinodb/trino#10551

Closed

	builder.protocols(Collections.singletonList(Protocol.HTTP_1_1));
	builder.protocols(ImmutableList.of(Protocol.HTTP_1_1));

Conversation

adonley commented Apr 17, 2021 • edited by tdcmeehan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amarkowitz commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amarkowitz commented May 24, 2021

Uh oh!

tdcmeehan commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adonley May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amarkowitz commented May 24, 2021

Uh oh!

pettyjamesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pettyjamesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pettyjamesm commented May 28, 2021

Uh oh!

adonley commented May 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdcmeehan commented May 31, 2021

Uh oh!

adonley commented Jun 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adonley commented Apr 17, 2021 •

edited by tdcmeehan

Loading

linux-foundation-easycla bot commented Apr 17, 2021 •

edited

Loading

amarkowitz commented May 13, 2021 •

edited

Loading

tdcmeehan commented May 24, 2021 •

edited

Loading

adonley May 25, 2021 •

edited

Loading

adonley commented May 29, 2021 •

edited

Loading