Skip to content

[BEAM-4425] Force load JDBC drivers#5503

Merged
kennknowles merged 2 commits intoapache:masterfrom
kennknowles:jdbc-force
May 29, 2018
Merged

[BEAM-4425] Force load JDBC drivers#5503
kennknowles merged 2 commits intoapache:masterfrom
kennknowles:jdbc-force

Conversation

@kennknowles
Copy link
Member

In some environments (known to be OSX) the JDBC drivers are not loaded by the time they are used. This is a cherry-pick from #5490 to make it easy for anyone to review just these commits.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

@kennknowles
Copy link
Member Author


@Before
public void before() throws Exception {
Class.forName("org.apache.beam.sdk.extensions.sql.impl.JdbcDriver");
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to understand the environments that require this. What runtime do they have? This should be handled by the service file for java.sql.Driver in Java 7+. How do we test that path keeps working?

Copy link
Member Author

Choose a reason for hiding this comment

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

If it varies on env, I think we'd need a test matrix. We used to have a bit but shut down our Travis job.

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO(me): when I am at the computers where it is broken, report my env to the JIRA.

@timrobertson100
Copy link
Contributor

I have not investigated at all, other than to apply the changes and run the tests.

I can confirm tests now pass on my environment (macbook pro, high Sierra 10.13.4, Java 1.8.0_172-b11).

Without them I saw:
java.sql.SQLException: No suitable driver found for jdbc:calcite
java.sql.SQLException: No suitable driver found for jdbc:beam

@apilloud
Copy link
Member

The tests pass without this change on Linux openjdk 1.8.0_131, OS X Sierra 10.12.6 with Java 1.8.0_141-b15, and OS X Sierra 10.12.6 upgraded to Java 1.8.0_172-b11.

@timrobertson100
Copy link
Contributor

timrobertson100 commented May 29, 2018

Interesting @apilloud - how are you launching the tests please to ensure we are equivalent?

./gradlew :beam-sdks-java-extensions-sql:test

@apilloud
Copy link
Member

I think that might be it. I am running this: ./gradlew :beam-sdks-java-extensions-sql:test

@apilloud
Copy link
Member

LGTM.

The thing this is suppose to be testing isn't working, but the tests pass anyway because a bunch of other things also load the JDBC driver. I can repro this if I ensure the JdbcDriverTest is the first thing run. That makes it an ordering issue.

@vectorijk
Copy link
Contributor

w/o loading Class.forName(""), it failed on my env. paste my env: command ./gradlew :beam-sdks-java-extensions-sql:test macOS, java 1.8.0_112

@kennknowles kennknowles merged commit 760ad59 into apache:master May 29, 2018
@kennknowles
Copy link
Member Author

OK, merging to fix build and we should track in JIRA that the service loader pattern isn't working.

@kennknowles kennknowles deleted the jdbc-force branch July 3, 2018 21:57
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.

4 participants