Add defer_connect config to allow eagerly verifying connection #394

Shaheer-rossoneri14 · 2023-07-18T18:36:31Z

Description

Add defer_connect config to allow eagerly verifying connection.

Depends on #393.

Non-technical explanation

This commit adds a new connection parameter defer_connect which can be
set to False to force creating a connection when trino.dbapi.connect
is called. Any connection errors as a result of that get rewrapped into
trino.exceptions.TrinoConnectionError.

This doesn't end up actually executing a query on the server because
after the initial POST request the nextUri in the response is not
followed which leaves the query in QUEUED state. This is not documented
in the Trino REST API but the server does behave like this today. The
benefit is that we can very cheaply verify if the connection is valid
without polluting the server's query history or adding queries to queue.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

* Fix some things. ({issue}`issuenumber`)

cla-bot · 2023-07-18T18:36:34Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2023-07-18T18:59:47Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

hovaesco · 2023-07-23T14:02:29Z

Please drop 9d417d6 from here since it's being added here #393

Shaheer-rossoneri14 · 2023-08-06T15:16:42Z

@hovaesco Dropped the commit 9d417d6.

hashhar · 2023-08-17T05:19:03Z

This is useful since I've heard often that people complain that they don't know the credentials are wrong or that the server isn't running AFTER issuing the first query. However only PyMySql client seems to use the defer_connect arg.

hashhar · 2023-08-17T05:20:03Z

@ebyhr @mdesmet @hovaesco what are your opinions? I like this because it's a relatively small change and addresses a complaint I've heard quite often. The downside is there are already enough parameters for the connect method.

hovaesco · 2023-08-17T08:16:02Z

It looks like a useful change from my perspective, note that PyMySql sets defer_connect to False by default.

hashhar · 2023-08-17T10:18:25Z

trino/dbapi.py

@@ -157,6 +159,7 @@ def __init__(
        legacy_prepared_statements=None,
        roles=None,
        timezone=None,
+        defer_connect=True,


Please change the default to False.

hashhar · 2023-08-17T10:19:07Z

trino/dbapi.py

+                    "error {}{}".format(
+                        test_response.status_code,
+                        ": {}".format(test_response.content)
+                        if test_response.content
+                        else "",


please simplify the format string, maybe using simple concatenation instead

right now it's using nested format like error {}{: {}}

hovaesco · 2023-08-31T08:45:35Z

Please rebase on master.

This commit adds a new connection parameter `defer_connect` which can be set to False to force creating a connection when `trino.dbapi.connect` is called. Any connection errors as a result of that get rewrapped into `trino.exceptions.TrinoConnectionError`. By default `defer_connect` is set to `True` so users can explicitly call `trino.dbapi.Connection.connect` to do the connection check. This doesn't end up actually executing a query on the server because after the initial POST request the nextUri in the response is not followed which leaves the query in QUEUED state. This is not documented in the Trino REST API but the server does behave like this today. The benefit is that we can very cheaply verify if the connection is valid without polluting the server's query history or adding queries to queue. Some unit tests today relied on the lazy connection behaviour so they have been adjusted accrodingly.

hashhar · 2023-09-04T13:02:46Z

trino/dbapi.py

+            verify=self._http_session.verify,
+        )
+        try:
+            test_response = connection_test_request.post("<not-going-to-be-executed>")


@electrum The server doesn't seem to "execute" the query until the first nextUri is followed. i.e. if you send just a POST /v1/statement and then do nothing then query execution doesn't happen.

Is this expected from the protocol? Asking because this change relies on this behaviour to allow verifying that connection is indeed correct - e.g. proper auth instead of failing when the first query is submitted.

This is a UX issue which people complain about when using the Trino CLI or JDBC as well for example.

I discussed this with David offline and he said that's it's expected behaviour.

However if we don't send a DELETE the query might occupy resources on the server. So in this change we should send a subsequent DELETE as well for the query.

Additionally we also discussed about making the protocol more "synchronous" in future so that connection verification happens eagerly always.

@Shaheer-rossoneri14 Can you adjust this code to send a DELETE for the fake query we executed as well.

hashhar · 2023-09-12T06:29:00Z

trino/dbapi.py

@@ -201,6 +204,31 @@ def __init__(
        self.legacy_primitive_types = legacy_primitive_types
        self.legacy_prepared_statements = legacy_prepared_statements

+        if not defer_connect:


Suggested change

if not defer_connect:

if not defer_connect and auth != constants.DEFAULT_AUTH:

@hovaesco This would mean we'll only do eager connection if some authentication is actually provided. WDYT? Or do you think existing code is better (simpler and we don't assume when someone might want eager connections)?

I don't see any production use cases when auth is constants.DEFAULT_AUTH but might be helpful in some local testing.

Shaheer-rossoneri14 force-pushed the Shaheer-rossoneri14/eager-connect branch from f561aa7 to 05695e8 Compare July 18, 2023 18:59

Shaheer-rossoneri14 force-pushed the Shaheer-rossoneri14/eager-connect branch from 05695e8 to 676c42a Compare August 6, 2023 15:13

cla-bot bot added the cla-signed label Aug 6, 2023

hashhar reviewed Aug 17, 2023

View reviewed changes

Shaheer-rossoneri14 force-pushed the Shaheer-rossoneri14/eager-connect branch from d716d01 to e764a6a Compare September 2, 2023 08:15

hashhar reviewed Sep 4, 2023

View reviewed changes

hashhar reviewed Sep 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add defer_connect config to allow eagerly verifying connection #394

Add defer_connect config to allow eagerly verifying connection #394

Shaheer-rossoneri14 commented Jul 18, 2023

cla-bot bot commented Jul 18, 2023

cla-bot bot commented Jul 18, 2023

hovaesco commented Jul 23, 2023

Shaheer-rossoneri14 commented Aug 6, 2023

hashhar commented Aug 17, 2023

hashhar commented Aug 17, 2023

hovaesco commented Aug 17, 2023

hashhar Aug 17, 2023

hashhar Aug 17, 2023

hovaesco commented Aug 31, 2023

hashhar Sep 4, 2023

hashhar Sep 12, 2023

hashhar Sep 12, 2023

hovaesco Sep 12, 2023

	if not defer_connect:
	if not defer_connect and auth != constants.DEFAULT_AUTH:

Add defer_connect config to allow eagerly verifying connection #394

Are you sure you want to change the base?

Add defer_connect config to allow eagerly verifying connection #394

Conversation

Shaheer-rossoneri14 commented Jul 18, 2023

Description

Non-technical explanation

Release notes

cla-bot bot commented Jul 18, 2023

cla-bot bot commented Jul 18, 2023

hovaesco commented Jul 23, 2023

Shaheer-rossoneri14 commented Aug 6, 2023

hashhar commented Aug 17, 2023

hashhar commented Aug 17, 2023

hovaesco commented Aug 17, 2023

hashhar Aug 17, 2023

Choose a reason for hiding this comment

hashhar Aug 17, 2023

Choose a reason for hiding this comment

hovaesco commented Aug 31, 2023

hashhar Sep 4, 2023

Choose a reason for hiding this comment

hashhar Sep 12, 2023

Choose a reason for hiding this comment

hashhar Sep 12, 2023

Choose a reason for hiding this comment

hovaesco Sep 12, 2023

Choose a reason for hiding this comment