Skip to content

Fix connection URL example and link to Oracle docs#9044

Merged
hashhar merged 1 commit intotrinodb:masterfrom
jhlodin:jl/oracle-connection
Sep 10, 2021
Merged

Fix connection URL example and link to Oracle docs#9044
hashhar merged 1 commit intotrinodb:masterfrom
jhlodin:jl/oracle-connection

Conversation

@jhlodin
Copy link
Copy Markdown
Contributor

@jhlodin jhlodin commented Aug 30, 2021

Address an incorrect connection-url example, and link to the corresponding Oracle doc for more info.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Both are correct. Depends on how you want to connect to Oracle and what version of Oracle you are on.

Quoting the link you shared:

Starting Oracle Database 10g, Oracle Service IDs are not supported.

This is the one we use in our existing example.

Oracle also supports a lot of other ways (all mentioned in the linked document) but it's important to note:

Thin-style Service Name Syntax
jdbc:oracle:thin:scott/tiger@//myhost:1521/myservicename

This is the one you are adding now.

So IMO somehow we should make it more obvious that the example is just that - an example. People should consult the documentation to see whatever URL they should use.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My understanding of documentation (https://docs.oracle.com/cd/B28359_01/java.111/b31224/urls.htm#BEIDHCBA) is that these are correct forms:

  • connection-url=jdbc:oracle:thin:@example.net:1521:ORCLCDB
  • connection-url=jdbc:oracle:thin:@//example.net:1521/ORCLCDB

connection-url=jdbc:oracle:thin:@example.net:1521/ORCLCDB is not correct

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But 12.x documentation here https://docs.oracle.com/en/database/oracle/oracle-database/21/tdpjd/getting-started.html#GUID-D1652CF5-5A80-40BF-BB96-016D1694BE5A gives example:

Connection connection =
        DriverManager.getConnection("jdbc:oracle:thin:@localhost:1522/xepdb1", "hr", "hr");

So I guess all forms are correct...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given all that we should probably have multiple examples and links to the JDBC driver docs and explain that the URL just has to be valid for the driver use and the connected Oracle database from Trino..

Copy link
Copy Markdown
Contributor Author

@jhlodin jhlodin Aug 31, 2021

Choose a reason for hiding this comment

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

Followup question, the requirements section above says that you need Oracle 12 or higher, so should we even be documenting v11? I don't think we should provide example connection URLs for versions we say we don't support, a generic "refer to the Oracle docs for more information" should suffice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should not imho

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mosabua The existing example is present in 12g documentation too - the last link that Pawel shared.

We can have multiple examples but it can't possibly be exhaustive. If left to me I'd just add a comment in the example config (before the connection-url line) that says to consult the Oracle documentation for the URL to use.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated, please have a look to see if that's a good compromise.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What changed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I remembered how to use git, apparently. 😅

Changes are there now.

@jhlodin jhlodin force-pushed the jl/oracle-connection branch from e4c96f4 to 3d1573a Compare September 2, 2021 19:50
@cla-bot cla-bot bot added the cla-signed label Sep 2, 2021
@jhlodin jhlodin force-pushed the jl/oracle-connection branch from 3d1573a to e1a6a57 Compare September 3, 2021 13:50
Copy link
Copy Markdown
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.

Looks good to me now. Much better for people who are new to Oracle and liable to copy-pasting things.

@hashhar hashhar requested a review from mosabua September 3, 2021 17:37
@hashhar hashhar merged commit 11627c8 into trinodb:master Sep 10, 2021
@hashhar hashhar added this to the 362 milestone Sep 10, 2021
@jhlodin jhlodin deleted the jl/oracle-connection branch September 10, 2021 13:50
v-jizhang added a commit to v-jizhang/presto that referenced this pull request Apr 25, 2022
Cherry-pick of trinodb/trino#9044
and
Cherry-pick of trinodb/trino#11656

Co-authored-by: Joe Lodin <joe.lodin@starburstdata.com>
highker pushed a commit to prestodb/presto that referenced this pull request Apr 25, 2022
Cherry-pick of trinodb/trino#9044
and
Cherry-pick of trinodb/trino#11656

Co-authored-by: Joe Lodin <joe.lodin@starburstdata.com>
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.

5 participants