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

VitaliiMaltsev · 2022-11-10T16:52:12Z

What

Fix for https://app.zenhub.com/workspaces/db--dw-source-connectors-6333360e0a41155061efbcbd/issues/airbytehq/airbyte-cloud/3063

How

use native Postgres connection timeout if it's not set by the user

Before

After

🚨 User Impact 🚨

none

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

VitaliiMaltsev · 2022-11-10T16:53:43Z

/test connector=connectors/source-postgres-strict-encrypt

🕑 connectors/source-postgres-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/3438769222
✅ connectors/source-postgres-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/3438769222
No Python unittests run

Build Passed

Test summary info:

All Passed

VitaliiMaltsev · 2022-11-10T16:53:50Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/3438770585
❌ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/3438770585
🐛 https://gradle.com/s/mg2djwct54f2k

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestSpec::test_enum_usage[inputs0] - TypeError: list ind...
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestConnection.test_check: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestDiscovery.test_discover: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestBasicRead.test_read: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestFullRefresh.test_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
============ 1 failed, 12 passed, 5 skipped, 20 warnings in 32.71s =============

docs/integrations/sources/postgres.md

airbyte-db/db-lib/src/main/java/io/airbyte/db/factory/DataSourceFactory.java

akashkulk · 2022-11-15T01:07:04Z

airbyte-db/db-lib/src/main/java/io/airbyte/db/factory/DataSourceFactory.java

-              : CONNECT_TIMEOUT_DEFAULT;
-      if (connectionTimeout.getSeconds() == 0) {
-        return connectionTimeout.toMillis();
+    private static long getConnectionTimeoutMs(final Map<String, String> connectionProperties, String driverClassName) {


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

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.

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

ryankfu · 2022-11-15T01:10:20Z

airbyte-db/db-lib/src/main/java/io/airbyte/db/factory/DataSourceFactory.java

      } else {
-        return (connectionTimeout.compareTo(CONNECT_TIMEOUT_DEFAULT) > 0 ? connectionTimeout : CONNECT_TIMEOUT_DEFAULT).toMillis();
+        return CONNECT_TIMEOUT_DEFAULT.toMillis();


If I'm understanding this correctly this changes the default behavior for all other connectors where the user can pass in a connection_timeout property. In the javadoc description, we allow for users to set the connection_timeout to 0 which means timeout duration is infinity

This seems to change that behavior for all connectors that isn't postgres. Also as a note the reason why default of 60 seconds was chosen in the past was due to integration tests failing on the default of 30 seconds (also in the javadoc description). If that has changed, then updating the javadoc comment to match makes sense

If I'm understanding this correctly this changes the default behavior for all other connectors where the user can pass in a connection_timeout property. In the javadoc description, we allow for users to set the connection_timeout to 0 which means timeout duration is infinity

This seems to change that behavior for all connectors that isn't postgres. Also as a note the reason why default of 60 seconds was chosen in the past was due to integration tests failing on the default of 30 seconds (also in the javadoc description). If that has changed, then updating the javadoc comment to match makes sense

@ryankfu good point
changed logic on
if (driverClassName.equals(DatabaseDriver.POSTGRESQL.getDriverClassName())) { new behaviour ; } else { old behaviour ; }

…urces creation

…meouts' into vmaltsev/3063-postgres-source-timeouts

VitaliiMaltsev · 2022-11-15T10:55:47Z

/test connector=connectors/source-postgres-strict-encrypt

🕑 connectors/source-postgres-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/3469788558
✅ connectors/source-postgres-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/3469788558
No Python unittests run

Build Passed

Test summary info:

All Passed

akashkulk

Changes look in general to me - could you run source Postgres acceptance tests once again and make sure that they pass before merging

ryankfu · 2022-11-15T17:40:15Z

airbyte-db/db-lib/src/main/java/io/airbyte/db/factory/DataSourceFactory.java

+
+      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


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

ryankfu

Overall looks good, there's some pmd fixes within the build that need to be addressed but once that's fixed should be good to merge

VitaliiMaltsev · 2022-11-16T12:52:33Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/3479500446
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/3479500446
Python tests coverage:

	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       139      5    96%   87, 93, 235, 239-240
	 source_acceptance_test/conftest.py                     196     92    53%   35, 41-43, 48, 54, 60, 66, 72-74, 93, 98-100, 106-108, 114-115, 120-121, 126, 132, 141-150, 156-161, 176, 200, 231, 237, 243-248, 256-261, 269-282, 287-293, 300-311, 318-334
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              394    111    72%   53, 58, 87-95, 100-107, 111-112, 116-117, 293, 331-348, 357-365, 369-374, 380, 413-418, 456-463, 506-508, 511, 576-584, 596-599, 604, 660-661, 667, 670, 706-716, 729-754
	 source_acceptance_test/tests/test_incremental.py       158     14    91%   52-59, 64-77, 240
	 source_acceptance_test/utils/asserts.py                 37      2    95%   57-58
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       112     50    55%   23-26, 32, 36, 39-68, 71-73, 76-78, 81-83, 86-88, 91-93, 96-114, 148-150
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1559    349    78%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestConnection.test_check: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestDiscovery.test_discover: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestBasicRead.test_read: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestFullRefresh.test_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
================= 14 passed, 5 skipped, 21 warnings in 27.22s ==================

VitaliiMaltsev · 2022-11-16T15:12:01Z

Overall looks good, there's some pmd fixes within the build that need to be addressed but once that's fixed should be good to merge

@akashkulk all tests passed #19291 (comment)

github-actions · 2022-11-16T15:47:04Z

Affected Connector Report

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to do the following as needed:

Run integration tests
Bump connector version
Add changelog
Publish the new version

⚠ Sources (3)

Connector	Version	Changelog	Publish
`source-alloydb`	`1.0.17`	✅	✅
`source-alloydb-strict-encrypt`	`1.0.17`	✅	⚠ (not in seed)
`source-postgres-strict-encrypt`	`1.0.24`	✅	⚠ (not in seed)

See "Actionable Items" below for how to resolve warnings and errors.

✅ Destinations (0)

Connector	Version	Changelog	Publish

See "Actionable Items" below for how to resolve warnings and errors.

Actionable Items

(click to expand)

Category	Status	Actionable Item
Version	❌ mismatch	The version of the connector is different from its normal variant. Please bump the version of the connector.
	⚠ doc not found	The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like `source-jdbc` is not published or documented). Please double-check to make sure that it is not a bug.
Changelog	⚠ doc not found	The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like `source-jdbc` is not published or documented). Please double-check to make sure that it is not a bug.
	❌ changelog missing	There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog.
Publish	⚠ not in seed	The connector is not in the seed file (e.g. `source_definitions.yaml`), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug.
	❌ diff seed version	The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the `/publish` command to publish the latest version.

VitaliiMaltsev · 2022-11-16T15:51:08Z

/publish connector=connectors/source-postgres-strict-encrypt

🕑 Publishing the following connectors:
connectors/source-postgres-strict-encrypt
https://github.com/airbytehq/airbyte/actions/runs/3480909976

Connector	Did it publish?	Were definitions generated?
connectors/source-postgres-strict-encrypt	✅	❌

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

VitaliiMaltsev · 2022-11-16T15:51:22Z

/publish connector=connectors/source-postgres

🕑 Publishing the following connectors:
connectors/source-postgres
https://github.com/airbytehq/airbyte/actions/runs/3480911630

Connector	Did it publish?	Were definitions generated?
connectors/source-postgres	✅	✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

…ser (#19291) * Postgres Source: use native Postgres timeout if it's not set by the user * refactoring * updated connection timeout logic and added tests for different datasources creation * fixed pmd * refactoring * bump version * auto-bump connector version Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>

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

6817961

VitaliiMaltsev temporarily deployed to more-secrets November 10, 2022 16:53 Inactive

refactoring

01625a7

octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Nov 11, 2022

VitaliiMaltsev temporarily deployed to more-secrets November 11, 2022 09:47 Inactive

Merge branch 'master' into vmaltsev/3063-postgres-source-timeouts

0002cc1

VitaliiMaltsev requested review from bleonard, subodh1810 and rodireich November 11, 2022 10:00

VitaliiMaltsev marked this pull request as ready for review November 11, 2022 10:01

akashkulk reviewed Nov 15, 2022

View reviewed changes

docs/integrations/sources/postgres.md Outdated Show resolved Hide resolved

akashkulk reviewed Nov 15, 2022

View reviewed changes

airbyte-db/db-lib/src/main/java/io/airbyte/db/factory/DataSourceFactory.java Outdated Show resolved Hide resolved

akashkulk reviewed Nov 15, 2022

View reviewed changes

ryankfu reviewed Nov 15, 2022

View reviewed changes

VitaliiMaltsev added 2 commits November 15, 2022 12:54

updated connection timeout logic and added tests for different dataso…

a016852

…urces creation

Merge remote-tracking branch 'origin/vmaltsev/3063-postgres-source-ti…

21b1146

…meouts' into vmaltsev/3063-postgres-source-timeouts

VitaliiMaltsev temporarily deployed to more-secrets November 15, 2022 10:57 Inactive

VitaliiMaltsev requested review from ryankfu and akashkulk November 15, 2022 11:02

akashkulk reviewed Nov 15, 2022

View reviewed changes

ryankfu reviewed Nov 15, 2022

View reviewed changes

ryankfu approved these changes Nov 15, 2022

View reviewed changes

rodireich approved these changes Nov 15, 2022

View reviewed changes

fixed pmd

7df7e83

VitaliiMaltsev temporarily deployed to more-secrets November 16, 2022 10:31 Inactive

refactoring

3b6e70b

VitaliiMaltsev temporarily deployed to more-secrets November 16, 2022 10:35 Inactive

Merge branch 'master' into vmaltsev/3063-postgres-source-timeouts

e07e9e3

VitaliiMaltsev temporarily deployed to more-secrets November 16, 2022 11:07 Inactive

akashkulk approved these changes Nov 16, 2022

View reviewed changes

bump version

559d275

VitaliiMaltsev requested a review from a team as a code owner November 16, 2022 15:45

octavia-squidington-iv added area/connectors Connector related issues connectors/source/postgres connectors/source/postgres-strict-encrypt labels Nov 16, 2022

VitaliiMaltsev temporarily deployed to more-secrets November 16, 2022 15:47 Inactive

auto-bump connector version

bebdfc8

octavia-squidington-iii temporarily deployed to more-secrets November 16, 2022 17:29 Inactive

VitaliiMaltsev merged commit a1db24c into master Nov 16, 2022

VitaliiMaltsev deleted the vmaltsev/3063-postgres-source-timeouts branch November 16, 2022 17:49

github-actions bot mentioned this pull request Nov 17, 2022

Bump helm chart version reference to 0.40.44 #19546

Merged

octavia-squidington-iii mentioned this pull request Nov 18, 2022

Bump Airbyte version from 0.40.18 to 0.40.19 #19579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

VitaliiMaltsev commented Nov 10, 2022 •

edited

Loading

VitaliiMaltsev commented Nov 10, 2022 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Nov 10, 2022 •

edited by github-actions bot

Loading

akashkulk Nov 15, 2022

ryankfu Nov 15, 2022 •

edited

Loading

VitaliiMaltsev Nov 15, 2022

ryankfu Nov 15, 2022

VitaliiMaltsev Nov 15, 2022

VitaliiMaltsev commented Nov 15, 2022 •

edited by github-actions bot

Loading

akashkulk left a comment

ryankfu Nov 15, 2022

ryankfu left a comment •

edited

Loading

VitaliiMaltsev commented Nov 16, 2022 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Nov 16, 2022

github-actions bot commented Nov 16, 2022 •

edited

Loading

⚠ Sources (3)

✅ Destinations (0)

Actionable Items

VitaliiMaltsev commented Nov 16, 2022 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Nov 16, 2022 •

edited by github-actions bot

Loading

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

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

Conversation

VitaliiMaltsev commented Nov 10, 2022 • edited Loading

What

How

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

VitaliiMaltsev commented Nov 10, 2022 • edited by github-actions bot Loading

Build Passed

VitaliiMaltsev commented Nov 10, 2022 • edited by github-actions bot Loading

Build Failed

akashkulk Nov 15, 2022

Choose a reason for hiding this comment

ryankfu Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

VitaliiMaltsev Nov 15, 2022

Choose a reason for hiding this comment

ryankfu Nov 15, 2022

Choose a reason for hiding this comment

VitaliiMaltsev Nov 15, 2022

Choose a reason for hiding this comment

VitaliiMaltsev commented Nov 15, 2022 • edited by github-actions bot Loading

Build Passed

akashkulk left a comment

Choose a reason for hiding this comment

ryankfu Nov 15, 2022

Choose a reason for hiding this comment

ryankfu left a comment • edited Loading

Choose a reason for hiding this comment

VitaliiMaltsev commented Nov 16, 2022 • edited by github-actions bot Loading

Build Passed

VitaliiMaltsev commented Nov 16, 2022

github-actions bot commented Nov 16, 2022 • edited Loading

Affected Connector Report

⚠ Sources (3)

✅ Destinations (0)

Actionable Items

VitaliiMaltsev commented Nov 16, 2022 • edited by github-actions bot Loading

VitaliiMaltsev commented Nov 16, 2022 • edited by github-actions bot Loading

VitaliiMaltsev commented Nov 10, 2022 •

edited

Loading

VitaliiMaltsev commented Nov 10, 2022 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Nov 10, 2022 •

edited by github-actions bot

Loading

ryankfu Nov 15, 2022 •

edited

Loading

VitaliiMaltsev commented Nov 15, 2022 •

edited by github-actions bot

Loading

ryankfu left a comment •

edited

Loading

VitaliiMaltsev commented Nov 16, 2022 •

edited by github-actions bot

Loading

github-actions bot commented Nov 16, 2022 •

edited

Loading

VitaliiMaltsev commented Nov 16, 2022 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Nov 16, 2022 •

edited by github-actions bot

Loading