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.

Sign up for GitHub

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

Jump to bottom

Consistent handling of Trino uri properties #22687

Merged
merged 5 commits into from
Jul 17, 2024
Merged

Consistent handling of Trino uri properties #22687

merged 5 commits into from
Jul 17, 2024

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Jul 16, 2024

This change makes configuration consistent between JDBC and client libraries so they can be configured in the same way. Instead of keeping individual values in the TrinoUri fields, properties are kept in Properties map and decoded on access. When TrinoUri is constructed, typed properties are encoded before being added to Properties.

OkHttpClient is constructed from the TrinoUri in the single place to keep the client configuration consistent.

This comes with bunch of tests that ensure that CLI options are mapped to connection properties and all connection properties can be set via TrinoUriBuilder.

@cla-bot cla-bot bot added the cla-signed label Jul 16, 2024
@github-actions github-actions bot added the jdbc Relates to Trino JDBC driver label Jul 16, 2024
@wendigo wendigo requested a review from nineinchnick July 16, 2024 18:22
@wendigo wendigo force-pushed the serafin/refactor-client branch from 87bfc5a to a21764d Compare July 16, 2024 18:52
@wendigo wendigo requested a review from electrum July 16, 2024 18:53
@wendigo wendigo force-pushed the serafin/refactor-client branch 2 times, most recently from f3e02f6 to aade18a Compare July 16, 2024 20:39
@wendigo
Copy link
Contributor Author

wendigo commented Jul 16, 2024

This is how DBeaver sees driver properties now:

Screenshot 2024-07-16 at 22 46 37

wendigo
wendigo commented Jul 16, 2024
View reviewed changes
client/trino-cli/src/main/java/io/trino/cli/QueryRunner.java
this.debug = debug;

OkHttpClient.Builder builder = new OkHttpClient.Builder();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are moved to a HttpClientFactory which is a single place to construct a HttpClient used by the Client libraryThese are moved to a HttpClientFactory which is a single place to construct a HttpClient used by the Client library

client/trino-cli/src/test/java/io/trino/cli/TestClientOptions.java
@@ -319,6 +322,42 @@ public void testDuplicateExtraCredentialKey()
.hasMessage("Multiple entries with same key: test.token.foo=bar and test.token.foo=foo");
}

@Test
public void testAllClientOptionsHaveMappingToAConnectionProperty()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test ensures that ClientOptions fields are mapped to TrinoUri ConnectionProperties

@wendigo wendigo requested review from ebyhr, hashhar and losipiuk July 17, 2024 05:20
losipiuk
losipiuk approved these changes Jul 17, 2024
View reviewed changes
Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

Seems fine. But hard to review

@wendigo
Copy link
Contributor Author

wendigo commented Jul 17, 2024

@losipiuk i know :(

@wendigo wendigo force-pushed the serafin/refactor-client branch 2 times, most recently from 219ed78 to 3d71ab9 Compare July 17, 2024 08:53
wendigo added 5 commits July 17, 2024 10:56
@wendigo
Consistent handling of Trino uri properties
8a1f845 
This change makes configuration consistent between JDBC and client libraries
so they can be configured in the same way. Instead of keeping individual values
in the TrinoUri fields, properties are kept in `Properties` map and decoded on access.
When `TrinoUri` is constructed, typed properties are encoded before being added to `Properties`.

`OkHttpClient` is constructed from the `TrinoUri` in the single place to keep the client configuration consistent.
@wendigo
Sort PropertyName values
97013b5
@wendigo
Add support for Resource Estimates to JDBC properties
e6ecb9f
@wendigo
Remove not needed builder call
a255def 
This property is already set through the uri
@wendigo
Make Trino client library agnostic of the http library
df65c98 
This will allow easier switching of the underlying http library in the future
@wendigo wendigo force-pushed the serafin/refactor-client branch from 3d71ab9 to df65c98 Compare July 17, 2024 08:56
@wendigo wendigo merged commit 0305dd9 into master Jul 17, 2024
108 checks passed
@wendigo wendigo deleted the serafin/refactor-client branch July 17, 2024 09:45
@github-actions github-actions bot added this to the 453 milestone Jul 17, 2024
@colebow colebow mentioned this pull request Jul 17, 2024
Merged
@colebow colebow added the no-release-notes This pull request does not require release notes entry label Jul 17, 2024
This was referenced Sep 5, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@losipiuk losipiuk

@nineinchnick nineinchnick

@electrum electrum

@ebyhr ebyhr

@hashhar hashhar

Assignees
No one assigned
Labels
cla-signed jdbc Relates to Trino JDBC driver no-release-notes This pull request does not require release notes entry
Milestone
453
Development

Successfully merging this pull request may close these issues.
3 participants
@wendigo @losipiuk @colebow