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.

Sign up for GitHub

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

JDBC Sources: Wrap SQLTransientException with ConfigErrorException #19711

Merged
merged 15 commits into from
Dec 1, 2022
Merged

JDBC Sources: Wrap SQLTransientException with ConfigErrorException #19711

Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f4df336
JDBC Sources: Wrap SQLTransientException with ConfigErrorException
VitaliiMaltsev Nov 22, 2022
e452097
JDBC Sources: Wrap SQLTransientException with ConfigErrorException
VitaliiMaltsev Nov 22, 2022
437d013
Merge branch 'master' into vmaltsev/331-mysql-config-error
VitaliiMaltsev Nov 22, 2022
b24ab14
removed unneeded exception handling in check method
VitaliiMaltsev Nov 23, 2022
7bdcbeb
Merge remote-tracking branch 'origin/vmaltsev/331-mysql-config-error'…
VitaliiMaltsev Nov 23, 2022
2266169
Merge branch 'master' into vmaltsev/331-mysql-config-error
VitaliiMaltsev Nov 23, 2022
99cd2d8
updated exception's message
VitaliiMaltsev Nov 29, 2022
b4c8056
Merge remote-tracking branch 'origin/vmaltsev/331-mysql-config-error'…
VitaliiMaltsev Nov 29, 2022
d520524
Merge branch 'master' into vmaltsev/331-mysql-config-error
VitaliiMaltsev Nov 29, 2022
9918bfc
Merge branch 'master' into vmaltsev/331-mysql-config-error
VitaliiMaltsev Nov 30, 2022
7161efd
add catch clause to avoid PMD rule violation
VitaliiMaltsev Nov 30, 2022
f88f7b4
Merge remote-tracking branch 'origin/vmaltsev/331-mysql-config-error'…
VitaliiMaltsev Nov 30, 2022
8f402b7
add original exception to the Throwable
VitaliiMaltsev Nov 30, 2022
5a56eaa
add catch clause
VitaliiMaltsev Dec 1, 2022
acbded7
Merge branch 'master' into vmaltsev/331-mysql-config-error
VitaliiMaltsev Dec 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/DefaultJdbcDatabase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package io.airbyte.db.jdbc;

import com.google.errorprone.annotations.MustBeClosed;
import io.airbyte.commons.exceptions.ConfigErrorException;
import io.airbyte.commons.exceptions.ConnectionErrorException;
import io.airbyte.commons.functional.CheckedConsumer;
import io.airbyte.commons.functional.CheckedFunction;
Expand All @@ -14,6 +15,7 @@
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.SQLTransientException;
import java.util.List;
import java.util.Objects;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -79,6 +81,12 @@ public DatabaseMetaData getMetaData() throws SQLException {
final DatabaseMetaData metaData = connection.getMetaData();
return metaData;
} catch (final SQLException e) {
if (e instanceof SQLTransientException) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. thank you for explaining!

}
}
// Some databases like Redshift will have null cause
if (Objects.isNull(e.getCause()) || !(e.getCause() instanceof SQLException)) {
throw new ConnectionErrorException(e.getSQLState(), e.getErrorCode(), e.getMessage(), e);
Expand Down