Fail gracefully if no JDBC driver found #1434
Conversation
rnorth
changed the title
modules/jdbc/src/main/java/org/testcontainers/containers/JdbcDatabaseContainer.java
Outdated
Show resolved
Hide resolved
modules/jdbc/src/main/java/org/testcontainers/containers/JdbcDatabaseContainer.java
Outdated
Show resolved
Hide resolved
modules/jdbc/src/test/java/org/testcontainers/jdbc/MissingJdbcDriverTest.java
Outdated
Show resolved
Hide resolved
|
Oh boy, awaitility has a dependency on hamcrest, which seems to break spectacularly when shaded. Having Hamcrest as an unshaded dependency doesn't seem good. Will pause work on this for today. |
rnorth
changed the title
rnorth
dismissed
kiview’s stale review
Taking this back to WIP state - I want to remove the awaitility dependency
(Auto-selected driver on classpath seems to be MariaDB; seemingly not compatible with 8.0.16 of MySQL)
to avoid reconnection attempts to dead containers after test has already completed
| @@ -26,7 +31,16 @@ DataSource getDataSource(JdbcDatabaseContainer container) { | |||
| hikariConfig.setJdbcUrl(container.getJdbcUrl()); | |||
| hikariConfig.setUsername(container.getUsername()); | |||
| hikariConfig.setPassword(container.getPassword()); | |||
| hikariConfig.setDriverClassName(container.getDriverClassName()); | |||
There was a problem hiding this comment.
I found that, seemingly dependent on test order, the MariaDB driver was incorrectly being used for communicating with MySQL. This was OK normally but failed with MySQL 8.0.16. This was a strange issue that only affected my local Gradle builds, not IDE runs of the affected test, which is why I think it's likely to be test order related.
Adding this line ensures that Hikari definitely uses the MySQL driver for talking to MySQL, and resolves the issue.
| return new HikariDataSource(hikariConfig); | ||
| @After | ||
| public void teardown() { | ||
| datasourcesForCleanup.forEach(HikariDataSource::close); |
There was a problem hiding this comment.
I was noticing a lot of Creating connection logs from the Clickhouse driver during unrelated tests, long after the Clickhouse tests completed.
I realised that the Hikari connection pools were not being closed after use, and were hanging around trying to reconnect to containers that were long dead.
I've added this cleanup code to the parent test class to ensure that the connection pools are always closed, regardless of how they are used. We could alternatively use try-with-resources, but we'd have to make sure we use it in all the tests that are based on this abstract class.
| logger().info("Waiting for database connection to become available at {} using query '{}'", getJdbcUrl(), getTestQueryString()); | ||
| Unreliables.retryUntilSuccess(getStartupTimeoutSeconds(), TimeUnit.SECONDS, () -> { |
There was a problem hiding this comment.
The changes in this class are the core of this PR:
I've removed use of duct-tape Unreliables as it isn't a great fit for the fail-fast behaviour we want. Also, Unreliables' currently can't properly enforce timeout on code that would otherwise infinitely loop (TLDR, it doesn't cancel the Future, which is a bug. I'd like to fix this separately).
I originally dropped in Awaitility as a replacement, but realised this was a mistake when shading it; it has a dependency on Hamcrest which seems to break when shaded, and I generally became quite uncomfortable with having Hamcrest in our dependency tree at all.
Despite not using a library any more, I hope that the code here is actually fairly clear in its intent.
rnorth
changed the title
|
Ready for re-review. Making the |
modules/jdbc/src/main/java/org/testcontainers/containers/JdbcDatabaseContainer.java
Outdated
Show resolved
Hide resolved
modules/jdbc/src/main/java/org/testcontainers/containers/JdbcDatabaseContainer.java
Outdated
Show resolved
Hide resolved
modules/jdbc/src/main/java/org/testcontainers/containers/JdbcDatabaseContainer.java
Outdated
Show resolved
Hide resolved
Fixes #678