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

Postgres Source: use native Postgres timeout if it's not set by the user #19291

Merged
merged 10 commits into from
Nov 16, 2022
4 changes: 4 additions & 0 deletions airbyte-db/db-lib/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ dependencies {

// MongoDB
implementation 'org.mongodb:mongodb-driver-sync:4.3.0'

// MySQL
implementation 'mysql:mysql-connector-java:8.0.30'

}

task(newConfigsMigration, dependsOn: 'classes', type: JavaExec) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@

package io.airbyte.db.factory;

import static org.postgresql.PGProperty.CONNECT_TIMEOUT;

import com.google.common.base.Preconditions;
import com.zaxxer.hikari.HikariConfig;
import com.zaxxer.hikari.HikariDataSource;
import java.io.Closeable;
import java.time.Duration;
import java.util.Map;
import java.util.Objects;
import javax.sql.DataSource;

/**
Expand Down Expand Up @@ -61,7 +64,7 @@ public static DataSource create(final String username,
.withJdbcUrl(jdbcConnectionString)
.withPassword(password)
.withUsername(username)
.withConnectionTimeoutMs(DataSourceBuilder.getConnectionTimeoutMs(connectionProperties))
.withConnectionTimeoutMs(DataSourceBuilder.getConnectionTimeoutMs(connectionProperties, driverClassName))
.build();
}

Expand Down Expand Up @@ -196,12 +199,23 @@ private DataSourceBuilder() {}
*
* @param connectionProperties custom jdbc_url_parameters containing information on connection
* properties
* @param driverClassName name of the JDBC driver
* @return DataSourceBuilder class used to create dynamic fields for DataSource
*/
private static long getConnectionTimeoutMs(final Map<String, String> connectionProperties) {
private static long getConnectionTimeoutMs(final Map<String, String> connectionProperties, String driverClassName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for this new functionality in DataSourceFactoryTest.java?

Copy link
Contributor

@ryankfu ryankfu Nov 15, 2022

Choose a reason for hiding this comment

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

It appears these changes were caught within the build failures

To add onto what Akash said, since this is a postgres specific feature I would add a test primarily for postgres and keep a generic one to account for non-postgres behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears these changes were caught within the build failures

To add onto what Akash said, since this is a postgres specific feature I would add a test primarily for postgres and keep a generic one to account for non-postgres behavior.

@akashkulk @ryankfu added tests for Postgres and MySQL to see the difference

// TODO: the usage of CONNECT_TIMEOUT is Postgres specific, may need to extend for other databases
if (driverClassName.equals(DatabaseDriver.POSTGRESQL.getDriverClassName())) {
final String pgPropertyConnectTimeout = CONNECT_TIMEOUT.getName();
// If the PGProperty.CONNECT_TIMEOUT was set by the user, then take its value, if not take the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: would remove the comment on #L206 and move this comment to above the if block for the Postgres logic. Otherwise changes look good

// default
if (connectionProperties.containsKey(pgPropertyConnectTimeout)
&& (Long.parseLong(connectionProperties.get(pgPropertyConnectTimeout)) >= 0)) {
return Duration.ofSeconds(Long.parseLong(connectionProperties.get(pgPropertyConnectTimeout))).toMillis();
} else {
return Duration.ofSeconds(Long.parseLong(Objects.requireNonNull(CONNECT_TIMEOUT.getDefaultValue()))).toMillis();
}
}
final Duration connectionTimeout;
// TODO: the usage of CONNECT_TIMEOUT_KEY is Postgres specific, may need to extend for other
// databases
connectionTimeout =
connectionProperties.containsKey(CONNECT_TIMEOUT_KEY) ? Duration.ofSeconds(Long.parseLong(connectionProperties.get(CONNECT_TIMEOUT_KEY)))
: CONNECT_TIMEOUT_DEFAULT;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.testcontainers.containers.MySQLContainer;

/**
* Test suite for the {@link DataSourceFactory} class.
*/
class DataSourceFactoryTest extends CommonFactoryTest {
private static final String CONNECT_TIMEOUT = "connectTimeout";

static String database;
static String driverClassName;
Expand All @@ -45,7 +47,7 @@ static void setup() {
@Test
void testCreatingDataSourceWithConnectionTimeoutSetAboveDefault() {
final Map<String, String> connectionProperties = Map.of(
"connectTimeout", "61");
CONNECT_TIMEOUT, "61");
final DataSource dataSource = DataSourceFactory.create(
username,
password,
Expand All @@ -58,9 +60,9 @@ void testCreatingDataSourceWithConnectionTimeoutSetAboveDefault() {
}

@Test
void testCreatingDataSourceWithConnectionTimeoutSetBelowDefault() {
void testCreatingPostgresDataSourceWithConnectionTimeoutSetBelowDefault() {
final Map<String, String> connectionProperties = Map.of(
"connectTimeout", "30");
CONNECT_TIMEOUT, "30");
final DataSource dataSource = DataSourceFactory.create(
username,
password,
Expand All @@ -69,13 +71,31 @@ void testCreatingDataSourceWithConnectionTimeoutSetBelowDefault() {
connectionProperties);
assertNotNull(dataSource);
assertEquals(HikariDataSource.class, dataSource.getClass());
assertEquals(60000, ((HikariDataSource) dataSource).getHikariConfigMXBean().getConnectionTimeout());
assertEquals(30000, ((HikariDataSource) dataSource).getHikariConfigMXBean().getConnectionTimeout());
}

@Test
void testCreatingMySQLDataSourceWithConnectionTimeoutSetBelowDefault() {
try (MySQLContainer<?> mySQLContainer = new MySQLContainer<>("mysql:8.0")) {
mySQLContainer.start();
final Map<String, String> connectionProperties = Map.of(
CONNECT_TIMEOUT, "30");
final DataSource dataSource = DataSourceFactory.create(
mySQLContainer.getUsername(),
mySQLContainer.getPassword(),
mySQLContainer.getDriverClassName(),
mySQLContainer.getJdbcUrl(),
connectionProperties);
assertNotNull(dataSource);
assertEquals(HikariDataSource.class, dataSource.getClass());
assertEquals(60000, ((HikariDataSource) dataSource).getHikariConfigMXBean().getConnectionTimeout());
}
}

@Test
void testCreatingDataSourceWithConnectionTimeoutSetWithZero() {
final Map<String, String> connectionProperties = Map.of(
"connectTimeout", "0");
CONNECT_TIMEOUT, "0");
final DataSource dataSource = DataSourceFactory.create(
username,
password,
Expand All @@ -88,7 +108,7 @@ void testCreatingDataSourceWithConnectionTimeoutSetWithZero() {
}

@Test
void testCreatingDataSourceWithConnectionTimeoutNotSet() {
void testCreatingPostgresDataSourceWithConnectionTimeoutNotSet() {
final Map<String, String> connectionProperties = Map.of();
final DataSource dataSource = DataSourceFactory.create(
username,
Expand All @@ -98,7 +118,25 @@ void testCreatingDataSourceWithConnectionTimeoutNotSet() {
connectionProperties);
assertNotNull(dataSource);
assertEquals(HikariDataSource.class, dataSource.getClass());
assertEquals(60000, ((HikariDataSource) dataSource).getHikariConfigMXBean().getConnectionTimeout());
assertEquals(10000, ((HikariDataSource) dataSource).getHikariConfigMXBean().getConnectionTimeout());
}

@Test
void testCreatingMySQLDataSourceWithConnectionTimeoutNotSet() {
try (MySQLContainer<?> mySQLContainer = new MySQLContainer<>("mysql:8.0")) {
mySQLContainer.start();
final Map<String, String> connectionProperties = Map.of();
final DataSource dataSource = DataSourceFactory.create(
mySQLContainer.getUsername(),
mySQLContainer.getPassword(),
mySQLContainer.getDriverClassName(),
mySQLContainer.getJdbcUrl(),
connectionProperties);
assertNotNull(dataSource);
assertEquals(HikariDataSource.class, dataSource.getClass());
assertEquals(60000, ((HikariDataSource) dataSource).getHikariConfigMXBean().getConnectionTimeout());
}

}

@Test
Expand Down
1 change: 1 addition & 0 deletions docs/integrations/sources/postgres.md
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,7 @@ The root causes is that the WALs needed for the incremental sync has been remove

| Version | Date | Pull Request | Subject |
|:--------|:-----------|:-------------------------------------------------------------------------------------------------------------------|:---------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| 1.0.24 | 2022-11-07 | [19291](https://github.com/airbytehq/airbyte/pull/19291) | Default timeout is reduced from 1 min to 10sec |
| 1.0.23 | 2022-11-07 | [19025](https://github.com/airbytehq/airbyte/pull/19025) | Stop enforce SSL if ssl mode is disabled |
| 1.0.22 | 2022-10-31 | [18538](https://github.com/airbytehq/airbyte/pull/18538) | Encode database name |
| 1.0.21 | 2022-10-25 | [18256](https://github.com/airbytehq/airbyte/pull/18256) | Disable allow and prefer ssl modes in CDC mode |
Expand Down