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.

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

Add a 2020 version of the JDBC post #7

Merged
merged 3 commits into from
Dec 30, 2020
Merged

Add a 2020 version of the JDBC post #7

merged 3 commits into from
Dec 30, 2020

Conversation

xhochy
Copy link
Owner

@xhochy xhochy commented Dec 27, 2020

@xhochy xhochy mentioned this pull request Dec 27, 2020
Copy link

@Thrameos Thrameos left a comment

Choose a reason for hiding this comment

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

Minor suggestions.

Comment on lines 108 to 109
classpath = os.path.join(os.getcwd(), "all-jar/target/drill-odbc-0.2-SNAPSHOT-jar-with-dependencies.jar")
jpype.startJVM(jpype.getDefaultJVMPath(), f"-Djava.class.path={classpath}")

Choose a reason for hiding this comment

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

This is still using a fairly old pattern. Directly setting up the classpath is still acceptable, but has the risk that if you need more than once class element you need to use the system separator which is different between windows and linux.

I would recommend the the code be changed to

classpath = os.path.join(os.getcwd(), "all-jar/target/drill-odbc-0.2-SNAPSHOT-jar-with-dependencies.jar")
jpype.startJVM(classpath=classpath)

The classpath argument can either be a string or a list of strings. Wildcards are accepted. This will automatically handle the separator if more than one is provided.

The other option is to use addClassPath. You can call addClassPath either before or after the startJVM statement which is great if you already have the JVM open and want to then add the library to it after. There may still be some interactions on this as I haven't tested it extensively with jdbc.

Copy link
Owner Author

Choose a reason for hiding this comment

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

addClassPath didn't work after the JVM was started. Then the driver couldn't be found, I'll open an issue with a reproducible example.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Comment on lines +153 to +154
java.lang.NoClassDefFoundError: java.lang.NoClassDefFoundError: oadd/org/apache/drill/exec/store/StoragePluginRegistry
```

Choose a reason for hiding this comment

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

JDBC has some strange behaviors as they do stuff with reflection that causes loading problems. I haven't seen this one yet though I suspect it would be resolved with a JClass statement before the connection is established. I would recommend putting in a ticket so that this one doesn't get lost.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Adding a JClass statement didn't help, I'll also open a reproducible issue.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@xhochy xhochy merged commit e87abf7 into master Dec 30, 2020
@xhochy xhochy deleted the jdbc-post branch December 30, 2020 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants