JDBC Sources: Wrap SQLTransientException with ConfigErrorException #19711

VitaliiMaltsev · 2022-11-22T14:03:51Z

What

Fixes https://app.zenhub.com/workspaces/db--dw-source-connectors-6333360e0a41155061efbcbd/issues/airbytehq/alpha-beta-issues/331

How

Wrap SQLTransientException with ConfigErrorException

🚨 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.

github-actions · 2022-11-22T14:05:09Z

Affected Connector Report

The latest commit has removed all connector-related changes. There are no more dependent connectors for this PR.

akashkulk · 2022-11-22T16:39:07Z

...elational-db/src/main/java/io/airbyte/integrations/source/relationaldb/AbstractDbSource.java

@@ -86,6 +86,11 @@ public AirbyteConnectionStatus check(final JsonNode config) throws Exception {
      }

      return new AirbyteConnectionStatus().withStatus(Status.SUCCEEDED);
+    } catch (final ConfigErrorException configErrorException) {


There isn't a need to catch and handle this exception here - it's handled at the top-level in https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/IntegrationRunner.java#L155

akashkulk

Looks good! Just a small comment there with respect to removing code from AbstractDbSource.java. Have you been able to test this?

… into vmaltsev/331-mysql-config-error

VitaliiMaltsev · 2022-11-23T11:46:42Z

Looks good! Just a small comment there with respect to removing code from AbstractDbSource.java. Have you been able to test this?

@akashkulk removed unneeded code from AbstractDbSource. Yes, i tested it on my localhost. Please take a look at screenshot in the description of this PR

rodireich · 2022-11-28T21:14:59Z

airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/DefaultJdbcDatabase.java

+      if (e instanceof SQLTransientException) {
+        final String message = e.getMessage();
+        if (message.contains("request timed out")) {
+          throw new ConfigErrorException("Connection timed out. Unable to contact server.");


Because it's a transient error, I wonder if adding something like "Please try again later" is appropriate,
and also maybe making use of the original e.getMessage() can add valuable information to the user.

Because it's a transient error, I wonder if adding something like "Please try again later" is appropriate, and also maybe making use of the original e.getMessage() can add valuable information to the user.

@rodireich the connection timeout can be not only because there is some problem with the existing server, but also because if the user pushed the wrong host / port on UI which will also lead to a timeout, so just adding "Please try again later" will be not entirely correct. Might be better to use - "Please check your server settings or try again later". What do you think about it?
Replacing the original e.getMessage() with a more user-friendly one is is a task of this issue. Please see this comment https://github.com/airbytehq/alpha-beta-issues/issues/331#issuecomment-1317496296

I see. thank you for explaining!

… into vmaltsev/331-mysql-config-error

VitaliiMaltsev · 2022-11-30T12:32:11Z

Looks good! Just a small comment there with respect to removing code from AbstractDbSource.java. Have you been able to test this?

@akashkulk removed unneeded code from AbstractDbSource. Yes, i tested it on my localhost. Please take a look at screenshot in the description of this PR

@akashkulk seems like we can not remove this catch clause cause it will lead to build failing. Please take a look here https://github.com/airbytehq/airbyte/actions/runs/3582926328/jobs/6027754532#step:16:1194

akashkulk · 2022-11-30T17:00:04Z

How is this breaking the build? I suspect it's related to integration tests failing. The build was also broken earlier this week, so maybe you can update your branch and that will be fixed.

If it's a quick fix - could you remove all exception handling in check() in AbstractDbSource, since it's handled in IntegrationRunner. If not, you can add back the exception handling you had originally had in and the cleanup is tracked by this issue : https://github.com/airbytehq/airbyte-internal-issues/issues/1109.

bleonard · 2022-11-30T17:33:16Z

airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/DefaultJdbcDatabase.java

@@ -79,6 +81,12 @@ public DatabaseMetaData getMetaData() throws SQLException {
      final DatabaseMetaData metaData = connection.getMetaData();
      return metaData;
    } catch (final SQLException e) {
+      if (e instanceof SQLTransientException) {


The build error is linting. Make a separate catch statement

Task :airbyte-db:db-lib:pmdMain FAILED
/actions-runner/_work/airbyte/airbyte/airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/DefaultJdbcDatabase.java:84: AvoidInstanceofChecksInCatchClause: An instanceof check is being performed on the caught exception. Create a separate catch clause for this exception type.

edgao · 2022-12-02T16:55:15Z

is there any risk to reverting this PR? I think it broke the source-postgres and mysql integration tests, e.g. https://github.com/airbytehq/airbyte/actions/runs/3597794993/jobs/6059935439#step:12:3214 (#19985 (comment))

specifically:

PostgresJdbcSourceAcceptanceTest.testCheckIncorrectDataBaseFailure is asserting that the message contains State code: 3D000
That's no longer the case; the message is now Could not connect with provided configuration. Error: Connection timed out. Unable to contact server. Please check your server settings or try again later
The problem is that this condition is always true, because HikariPool wraps everything in a "connect timed out" exception

I verified locally that the test case succeeds after git revert 9736352.

…ption (#19711)" This reverts commit 9736352.

…ption (#19711)" (#20031) This reverts commit 9736352.

VitaliiMaltsev added 2 commits November 22, 2022 15:55

JDBC Sources: Wrap SQLTransientException with ConfigErrorException

f4df336

JDBC Sources: Wrap SQLTransientException with ConfigErrorException

e452097

octavia-squidington-iv added area/connectors Connector related issues connectors/source/relational-db labels Nov 22, 2022

VitaliiMaltsev temporarily deployed to more-secrets November 22, 2022 14:04 Inactive

VitaliiMaltsev marked this pull request as ready for review November 22, 2022 14:08

VitaliiMaltsev requested review from bleonard, sashaNeshcheret, akashkulk and rodireich November 22, 2022 14:08

akashkulk reviewed Nov 22, 2022

View reviewed changes

Merge branch 'master' into vmaltsev/331-mysql-config-error

437d013

VitaliiMaltsev temporarily deployed to more-secrets November 22, 2022 19:25 Inactive

VitaliiMaltsev temporarily deployed to more-secrets November 22, 2022 19:26 Inactive

akashkulk reviewed Nov 22, 2022

View reviewed changes

VitaliiMaltsev and others added 3 commits November 23, 2022 13:42

removed unneeded exception handling in check method

b24ab14

Merge remote-tracking branch 'origin/vmaltsev/331-mysql-config-error'…

7bdcbeb

… into vmaltsev/331-mysql-config-error

Merge branch 'master' into vmaltsev/331-mysql-config-error

2266169

octavia-squidington-iv removed the area/connectors Connector related issues label Nov 23, 2022

VitaliiMaltsev temporarily deployed to more-secrets November 23, 2022 11:45 Inactive

VitaliiMaltsev requested a review from akashkulk November 23, 2022 11:47

akashkulk approved these changes Nov 27, 2022

View reviewed changes

rodireich reviewed Nov 28, 2022

View reviewed changes

VitaliiMaltsev requested a review from rodireich November 29, 2022 08:03

rodireich approved these changes Nov 29, 2022

View reviewed changes

updated exception's message

99cd2d8

VitaliiMaltsev temporarily deployed to more-secrets November 30, 2022 11:38 Inactive

VitaliiMaltsev added 2 commits November 30, 2022 14:29

add catch clause to avoid PMD rule violation

7161efd

Merge remote-tracking branch 'origin/vmaltsev/331-mysql-config-error'…

f88f7b4

… into vmaltsev/331-mysql-config-error

octavia-squidington-iv added the area/connectors Connector related issues label Nov 30, 2022

VitaliiMaltsev temporarily deployed to more-secrets November 30, 2022 12:31 Inactive

VitaliiMaltsev requested a review from akashkulk November 30, 2022 12:32

add original exception to the Throwable

8f402b7

octavia-squidington-iv removed the area/connectors Connector related issues label Nov 30, 2022

VitaliiMaltsev temporarily deployed to more-secrets November 30, 2022 13:53 Inactive

bleonard reviewed Nov 30, 2022

View reviewed changes

add catch clause

5a56eaa

VitaliiMaltsev temporarily deployed to more-secrets December 1, 2022 09:47 Inactive

Merge branch 'master' into vmaltsev/331-mysql-config-error

acbded7

VitaliiMaltsev temporarily deployed to more-secrets December 1, 2022 10:29 Inactive

VitaliiMaltsev temporarily deployed to more-secrets December 1, 2022 14:44 Inactive

VitaliiMaltsev merged commit 9736352 into master Dec 1, 2022

VitaliiMaltsev deleted the vmaltsev/331-mysql-config-error branch December 1, 2022 15:25

edgao mentioned this pull request Dec 2, 2022

Source Postgres: Fix not being able to configure wal2json plugin #19985

Merged

github-actions bot mentioned this pull request Dec 2, 2022

Bump helm chart version reference to 0.42.4 #20029

Closed

akashkulk added a commit that referenced this pull request Dec 2, 2022

Revert "JDBC Sources: Wrap SQLTransientException with ConfigErrorExce…

cbcc144

…ption (#19711)" This reverts commit 9736352.

akashkulk added a commit that referenced this pull request Dec 2, 2022

Revert "JDBC Sources: Wrap SQLTransientException with ConfigErrorExce…

a3343ef

…ption (#19711)" (#20031) This reverts commit 9736352.

This was referenced Dec 6, 2022

Bump Airbyte version from 0.40.23 to 0.40.24 #20161

Closed

Bump Airbyte version from 0.40.23 to 0.40.24 #20206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDBC Sources: Wrap SQLTransientException with ConfigErrorException #19711

JDBC Sources: Wrap SQLTransientException with ConfigErrorException #19711

VitaliiMaltsev commented Nov 22, 2022 •

edited

Loading

github-actions bot commented Nov 22, 2022 •

edited

Loading

akashkulk Nov 22, 2022

akashkulk left a comment

VitaliiMaltsev commented Nov 23, 2022

rodireich Nov 28, 2022

VitaliiMaltsev Nov 29, 2022

rodireich Nov 29, 2022

VitaliiMaltsev commented Nov 30, 2022

akashkulk commented Nov 30, 2022 •

edited

Loading

bleonard Nov 30, 2022

edgao commented Dec 2, 2022

JDBC Sources: Wrap SQLTransientException with ConfigErrorException #19711

JDBC Sources: Wrap SQLTransientException with ConfigErrorException #19711

Conversation

VitaliiMaltsev commented Nov 22, 2022 • edited Loading

What

How

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

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

Affected Connector Report

akashkulk Nov 22, 2022

Choose a reason for hiding this comment

akashkulk left a comment

Choose a reason for hiding this comment

VitaliiMaltsev commented Nov 23, 2022

rodireich Nov 28, 2022

Choose a reason for hiding this comment

VitaliiMaltsev Nov 29, 2022

Choose a reason for hiding this comment

rodireich Nov 29, 2022

Choose a reason for hiding this comment

VitaliiMaltsev commented Nov 30, 2022

akashkulk commented Nov 30, 2022 • edited Loading

bleonard Nov 30, 2022

Choose a reason for hiding this comment

edgao commented Dec 2, 2022

VitaliiMaltsev commented Nov 22, 2022 •

edited

Loading

github-actions bot commented Nov 22, 2022 •

edited

Loading

akashkulk commented Nov 30, 2022 •

edited

Loading