Support passing a connection URL to Trino CLI #12587

nineinchnick · 2022-05-28T10:02:40Z

Description

Allow the Trino CLI to accept an URL parameter instead of a (sometimes long) list of options. URLs are parsed same as JDBC URLs, but they don't have the jdbc: prefix. If any parameters are set in the URL, they can't be also set as options. But they can be present in the config file because these are only treated as default values and can be overridden.

Is this change a fix, improvement, new feature, refactoring, or other?
new feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)
cli

How would you describe this change to a non-technical end user or system administrator?
Trino CLI can accept an URL parameter.

Related issues, pull requests, and links

Fixes Pass a connection URL to Trino CLI #12543

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:

# CLI
* Trino CLI now accepts a single URL with parameters as an alternative to passing command line arguments. ({issue}`12587`)

# JDBC
* Connection parameter validation errors are now actionable ({issue}`12587`)

nineinchnick

To the reviewers, I understand this is pretty big and I'll try to split it up into multiple smaller commits.

client/trino-cli/src/main/java/io/trino/cli/ClientOptions.java

client/trino-client/src/main/java/io/trino/client/uri/TrinoUri.java

nineinchnick · 2022-05-30T09:42:30Z

client/trino-cli/src/main/java/io/trino/cli/Console.java

+                .map(CommandLine.Model.OptionSpec::longestName)
+                .collect(Collectors.toList());
+
+        TrinoUri uri = clientOptions.getTrinoUri(providedOptions);


This has a nice side effect of always converting all CLI options into URLs that can be used as JDBC URLs. Would it be useful to log the resulting connection URL on a debug level?

If it won't expose the password I'm for it.

nineinchnick · 2022-05-30T09:43:50Z

To the reviewers, I understand this is pretty big and I'll try to split it up into multiple smaller commits.

Done. If there's anything else I can do to make this easier to review please let me know.

mosabua

Docs only need minor changes. From the behaviour side of things I am not sure we want the new behavior to be the default and hence the one we use in the documentation.

And importantly I would like confirmation that the old behavior will continue to work... and we will continue to ensure that with test coverage.

docs/src/main/sphinx/installation/cli.rst

nineinchnick · 2022-05-31T10:01:50Z

Docs only need minor changes. From the behaviour side of things I am not sure we want the new behavior to be the default and hence the one we use in the documentation.

Let's wait for others to chime in about this. I made it the default since it's shorter. Users won't have to think about the option name (was it server, url, address or host?) and generally should be easier to type.

And importantly I would like confirmation that the old behavior will continue to work... and we will continue to ensure that with test coverage.

Yes, no existing behavior should change and there are lots of tests for that. I added a few test cases with URLs.

willmostly · 2022-05-31T13:37:43Z

Useful feature for debugging @nineinchnick, looking forward to this!

mosabua · 2022-05-31T14:55:30Z

Docs only need minor changes. From the behaviour side of things I am not sure we want the new behavior to be the default and hence the one we use in the documentation.

Let's wait for others to chime in about this. I made it the default since it's shorter. Users won't have to think about the option name (was it server, url, address or host?) and generally should be easier to type.

My concern is that it is NOT easier to type for any real use case beyond the URL only. In fact you need to know how to format the JDBC URL string.

My other questions would be .. how much does this work if you combine JDBC string and other parameters. Are all parameters from both input methods combined somehow? What if there are collisions .. does on override the other? Which one wins out? We need to make sure we understand that and also document it.

nineinchnick · 2022-05-31T15:09:28Z

My concern is that it is NOT easier to type for any real use case beyond the URL only. In fact you need to know how to format the JDBC URL string.

I meant it's easier only in the most basic use case, trino --server https://some.host --catalog bigdata --schema foo vs trino https://some.host/bigdata/foo.

My other questions would be .. how much does this work if you combine JDBC string and other parameters. Are all parameters from both input methods combined somehow? What if there are collisions .. does on override the other? Which one wins out? We need to make sure we understand that and also document it.

You can combine options, start with the URL only and add more options instead of URL parameters. You can't specify the same parameter both in the URL and as a CLI option. There's one sentence clarifying this in the docs update.

nineinchnick · 2022-06-08T14:33:48Z

@electrum PTAL

client/trino-jdbc/src/main/java/io/trino/jdbc/PropertyName.java

client/trino-jdbc/src/main/java/io/trino/jdbc/AbstractConnectionProperty.java

client/trino-client/src/main/java/io/trino/client/ClientSession.java

client/trino-client/src/main/java/io/trino/client/uri/TrinoUri.java

client/trino-cli/src/test/java/io/trino/cli/TestClientOptions.java

client/trino-client/src/main/java/io/trino/client/uri/TrinoUri.java

nineinchnick · 2023-07-18T06:18:17Z

Rebased to include the new KerberosConstrainedDelegation parameter. I actually found and fixed one issue after the last rebase, where I missed hostnameInCertificate in one place. Maybe we're missing a test for this.

client/trino-jdbc/src/main/java/io/trino/jdbc/AbstractConnectionProperty.java

client/trino-jdbc/src/main/java/io/trino/jdbc/ConnectionProperties.java

client/trino-jdbc/src/main/java/io/trino/jdbc/TrinoDriverUri.java

client/trino-jdbc/src/main/java/io/trino/jdbc/ConnectionProperties.java

client/trino-jdbc/src/test/java/io/trino/jdbc/TestTrinoDriver.java

client/trino-jdbc/src/test/java/io/trino/jdbc/TestTrinoDriverUri.java

wendigo · 2023-07-18T09:00:00Z

client/trino-client/src/main/java/io/trino/client/uri/TrinoUri.java

+                sslUseSystemTrustStore.orElse(false));
+    }
+
+    public void setupClient(OkHttpClient.Builder builder)


in both of the usages we are passing new, fresh OkHttpClient.Builder here.

Can we instead do: public OkHttpClient.Builder toHttpClientBuilder() ?

wendigo · 2023-07-18T09:01:33Z

client/trino-cli/src/main/java/io/trino/cli/QueryRunner.java

-            this.sslSetup = OkHttpUtil::setupInsecureSsl;
+        OkHttpClient.Builder builder = new OkHttpClient.Builder();
+        try {
+            sslSetup = uri.getSetupSsl();


why do we need this sslSetup at all?

Isn't uri.setupClient(builder); already doing required SSL initialization?

mosabua

Just a whole bunch of minor wording improvements .. essentially though this is good to go. BIG change!

client/trino-cli/src/main/java/io/trino/cli/ClientOptions.java

client/trino-client/src/main/java/io/trino/client/uri/TrinoUri.java

client/trino-jdbc/src/test/java/io/trino/jdbc/TestTrinoDriverUri.java

testing/trino-product-tests/src/main/java/io/trino/tests/product/cli/TestTrinoLdapCli.java

wendigo · 2023-07-19T10:55:42Z

There is related failure in suite-ldap @nineinchnick:

tests               | Expecting throwable message:
tests               |   "Authentication using username/password requires SSL to be enabled"
tests               | to contain:
tests               |   "TLS/SSL is required for authentication with username and password"
tests               | but did not.

wendigo · 2023-07-19T12:57:37Z

@nineinchnick can you update release notes:

Trino CLI now accepts a single URL with parameters as an alternative to passing command line arguments.

wendigo · 2023-07-19T14:18:54Z

testing/trino-product-tests/src/main/resources/tempto-configuration.yaml

@@ -46,7 +46,7 @@ databases:
    jdbc_driver_class: ${databases.presto.jdbc_driver_class}
    jdbc_url: jdbc:trino://${databases.presto.host}:${databases.presto.port}/hive/tpcds
    jdbc_user: hive
-    jdbc_password: "***empty***"
+    jdbc_password: ""


mosabua

All good now. Lets ship this!

wendigo · 2023-07-19T17:50:53Z

All tests have passed. I have a small refactor around creating http client used by the jdbc and client but I’ll follow up in a separate PR. Merging this as-is for now. Thanks @nineinchnick for improving CLI and making it consistent with JDBC driver.

nineinchnick · 2023-07-19T18:08:35Z

BTW there's one last improvement to be done in JDBC error messages, when the value is being converted, for example from a string to a boolean, the error now looks like:

Connection property SSL value is invalid: 2

But we could say how the expected values look like, for example:

Connection property SSL value '2' is invalid, expected 'true' or 'false'

I'll follow up with another small PR.

cla-bot bot added the cla-signed label May 28, 2022

github-actions bot added docs jdbc Relates to Trino JDBC driver labels May 28, 2022

nineinchnick commented May 28, 2022

View reviewed changes

client/trino-cli/src/main/java/io/trino/cli/ClientOptions.java Show resolved Hide resolved

nineinchnick force-pushed the cli-urls branch from 4a8a6e9 to fe172e6 Compare May 28, 2022 10:21

nineinchnick commented May 28, 2022

View reviewed changes

client/trino-client/src/main/java/io/trino/client/uri/TrinoUri.java Show resolved Hide resolved

nineinchnick force-pushed the cli-urls branch 2 times, most recently from 6890ddd to 2110ae1 Compare May 30, 2022 09:39

nineinchnick commented May 30, 2022

View reviewed changes

nineinchnick requested review from wendigo, electrum, hashhar, findepi and mosabua May 30, 2022 09:44

findepi removed their request for review May 30, 2022 13:03

mosabua requested changes May 30, 2022

View reviewed changes

docs/src/main/sphinx/installation/cli.rst Outdated Show resolved Hide resolved

docs/src/main/sphinx/installation/cli.rst Outdated Show resolved Hide resolved

nineinchnick force-pushed the cli-urls branch from 2110ae1 to 2b922b2 Compare May 31, 2022 09:57

nineinchnick force-pushed the cli-urls branch 2 times, most recently from e292651 to bb0fbd0 Compare June 5, 2022 09:15

github-actions bot added the tests:hive label Jun 5, 2022

nineinchnick force-pushed the cli-urls branch from bb0fbd0 to 0b29cca Compare June 5, 2022 12:49

nineinchnick force-pushed the cli-urls branch from 0b29cca to 19c4f08 Compare June 9, 2022 12:07

MiguelWeezardo reviewed Jun 9, 2022

View reviewed changes

nineinchnick force-pushed the cli-urls branch from 19c4f08 to 2e80235 Compare June 9, 2022 15:42

nineinchnick force-pushed the cli-urls branch 2 times, most recently from ff31cda to 158310b Compare July 18, 2023 06:13

wendigo reviewed Jul 18, 2023

View reviewed changes

nineinchnick force-pushed the cli-urls branch 2 times, most recently from 9bbadd6 to 78f2845 Compare July 18, 2023 16:24

mosabua requested changes Jul 18, 2023

View reviewed changes

nineinchnick added 4 commits July 19, 2023 08:29

Use enums for property names in the JDBC URL

5ecf80c

Make JDBC URL properties defaults strongly typed

969bc37

Make JDBC validation errors more actionable

bbee436

Pass catalog and schema to client session as optionals

12142c5

nineinchnick force-pushed the cli-urls branch from 78f2845 to 9cf6aed Compare July 19, 2023 08:53

nineinchnick added 2 commits July 19, 2023 13:07

Move TrinoDriverUri to client package

b738e53

Support passing a connection URL to Trino CLI

b9a245c

nineinchnick force-pushed the cli-urls branch from 9cf6aed to 55da1fa Compare July 19, 2023 11:13

wendigo and others added 2 commits July 19, 2023 15:31

Simplify mapping logic

92e7af8

Improve the description of the source CLI param

4e95a57

nineinchnick force-pushed the cli-urls branch from 55da1fa to 4e95a57 Compare July 19, 2023 13:32

wendigo reviewed Jul 19, 2023

View reviewed changes

wendigo requested a review from mosabua July 19, 2023 14:28

wendigo approved these changes Jul 19, 2023

View reviewed changes

mosabua approved these changes Jul 19, 2023

View reviewed changes

wendigo merged commit 86217f1 into trinodb:master Jul 19, 2023
101 checks passed

nineinchnick deleted the cli-urls branch July 19, 2023 17:58

github-actions bot added this to the 423 milestone Jul 19, 2023

colebow mentioned this pull request Aug 2, 2023

Add Trino 423 release notes #18496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support passing a connection URL to Trino CLI #12587

Support passing a connection URL to Trino CLI #12587

nineinchnick commented May 28, 2022 •

edited

nineinchnick left a comment

nineinchnick May 30, 2022

MiguelWeezardo Jun 9, 2022

nineinchnick commented May 30, 2022

mosabua left a comment •

edited

nineinchnick commented May 31, 2022

willmostly commented May 31, 2022

mosabua commented May 31, 2022

nineinchnick commented May 31, 2022

nineinchnick commented Jun 8, 2022

nineinchnick commented Jul 18, 2023

wendigo Jul 18, 2023

wendigo Jul 18, 2023

mosabua left a comment

wendigo commented Jul 19, 2023 •

edited

wendigo commented Jul 19, 2023

wendigo Jul 19, 2023

mosabua left a comment

wendigo commented Jul 19, 2023

nineinchnick commented Jul 19, 2023

Support passing a connection URL to Trino CLI #12587

Support passing a connection URL to Trino CLI #12587

Conversation

nineinchnick commented May 28, 2022 • edited

Description

Related issues, pull requests, and links

Documentation

Release notes

nineinchnick left a comment

Choose a reason for hiding this comment

nineinchnick May 30, 2022

Choose a reason for hiding this comment

MiguelWeezardo Jun 9, 2022

Choose a reason for hiding this comment

nineinchnick commented May 30, 2022

mosabua left a comment • edited

Choose a reason for hiding this comment

nineinchnick commented May 31, 2022

willmostly commented May 31, 2022

mosabua commented May 31, 2022

nineinchnick commented May 31, 2022

nineinchnick commented Jun 8, 2022

nineinchnick commented Jul 18, 2023

wendigo Jul 18, 2023

Choose a reason for hiding this comment

wendigo Jul 18, 2023

Choose a reason for hiding this comment

mosabua left a comment

Choose a reason for hiding this comment

wendigo commented Jul 19, 2023 • edited

wendigo commented Jul 19, 2023

wendigo Jul 19, 2023

Choose a reason for hiding this comment

mosabua left a comment

Choose a reason for hiding this comment

wendigo commented Jul 19, 2023

nineinchnick commented Jul 19, 2023

nineinchnick commented May 28, 2022 •

edited

mosabua left a comment •

edited

wendigo commented Jul 19, 2023 •

edited