wuchong commented on a change in pull request #14387:
URL: https://github.com/apache/flink/pull/14387#discussion_r548357533
##########
File path:
flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/internal/options/JdbcOptions.java
##########
@@ -144,6 +152,7 @@ public Builder setDBUrl(String dbURL) {
* optional, Handle the SQL dialect of jdbc driver. If not set,
it will be infer by
* {@link JdbcDialects#get} from DB url.
*/
+
Review comment:
Remove empty line.
##########
File path:
flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/internal/connection/JdbcConnectionProvider.java
##########
@@ -29,4 +29,6 @@
Connection getConnection() throws Exception;
Connection reestablishConnection() throws Exception;
+
+ Boolean isConnectionValid() throws Exception;
Review comment:
Return `boolean`.
##########
File path:
flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/internal/options/JdbcOptions.java
##########
@@ -170,7 +179,8 @@ public JdbcOptions build() {
});
}
- return new JdbcOptions(dbURL, tableName, driverName,
username, password, dialect, parallelism);
+ return new JdbcOptions(dbURL, tableName, driverName,
username, password, dialect, parallelism, connectionCheckTimeoutSeconds);
+
Review comment:
Remove empty line.
##########
File path: docs/dev/table/connectors/jdbc.md
##########
@@ -148,6 +148,13 @@ Connector Options
<td>String</td>
<td>The JDBC password.</td>
</tr>
+ <tr>
+ <td><h5>connection.max-retry-timeout</h5></td>
+ <td>optional</td>
+ <td style="word-wrap: break-word;">60s</td>
+ <td>Duration</td>
+ <td>Maximum timeout between retries.</td>
Review comment:
Would be better to mention that the timeout should be in second
granularity and shouldn't be smaller than 1 second.
##########
File path:
flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java
##########
@@ -92,8 +99,13 @@ public JdbcConnectionOptionsBuilder withPassword(String
password) {
return this;
}
+ public JdbcConnectionOptionsBuilder
withConnectionCheckTimeoutSeconds(int connectionCheckTimeoutSeconds) {
Review comment:
Could you add a javadoc on this method? Because this is a public API.
##########
File path:
flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java
##########
@@ -35,16 +35,18 @@
protected final String url;
protected final String driverName;
+ protected final int connectionCheckTimeoutSeconds;
@Nullable
protected final String username;
@Nullable
protected final String password;
- protected JdbcConnectionOptions(String url, String driverName, String
username, String password) {
+ protected JdbcConnectionOptions(String url, String driverName, String
username, String password, int connectionCheckTimeoutSeconds) {
this.url = Preconditions.checkNotNull(url, "jdbc url is empty");
this.driverName = Preconditions.checkNotNull(driverName,
"driver name is empty");
this.username = username;
this.password = password;
+ this.connectionCheckTimeoutSeconds =
connectionCheckTimeoutSeconds;
Review comment:
Would be better to validate the seconds here again, because this is a
public API, users may directly use it in `JdbcSink`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
