ZihanLi58 commented on code in PR #3509:
URL: https://github.com/apache/gobblin/pull/3509#discussion_r876466415
##########
gobblin-utility/src/main/java/org/apache/gobblin/util/jdbc/DataSourceProvider.java:
##########
@@ -51,6 +57,15 @@ public class DataSourceProvider implements
Provider<DataSource> {
public DataSourceProvider(@Named("dataSourceProperties") Properties
properties) {
this.basicDataSource = new BasicDataSource();
this.basicDataSource.setDriverClassName(properties.getProperty(CONN_DRIVER,
DEFAULT_CONN_DRIVER));
+ // the validation query should work beyond mysql; still, to bypass for any
reason, heed directive
+ if (!Boolean.parseBoolean(properties.getProperty(SKIP_VALIDATION_QUERY,
"false"))) {
Review Comment:
Any reason we want to make this one configurable?
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/db/ServiceDatabaseProviderImpl.java:
##########
@@ -63,8 +67,10 @@ private synchronized void ensureDataSource() {
dataSource.setUsername(configuration.getUserName());
dataSource.setPassword(configuration.getPassword());
- // MySQL server can timeout a connection so we need to validate
connections.
-
dataSource.setValidationQuery(MysqlDataSourceUtils.QUERY_CONNECTION_IS_VALID_AND_NOT_READONLY);
+ // MySQL server can timeout a connection so need to validate connections
before use
+ final String validationQuery =
MysqlDataSourceUtils.QUERY_CONNECTION_IS_VALID_AND_NOT_READONLY;
+ LOG.info("setting `DataSource` validation query: '" + validationQuery +
"'");
+ dataSource.setValidationQuery(validationQuery);
dataSource.setValidationQueryTimeout(5);
Review Comment:
Seems like it only set the setTestOnCreate() to be true, but if we don't
config max lifetime which indicate it's infinite, will there be a problem?
##########
gobblin-utility/src/main/java/org/apache/gobblin/util/jdbc/MysqlDataSourceUtils.java:
##########
@@ -15,7 +15,7 @@
* limitations under the License.
*/
-package org.apache.gobblin.metastore.util;
+package org.apache.gobblin.util.jdbc;
Review Comment:
I understand the rename here, but seems after this change this will be
inconsistent with the location. Should we move this class to another module?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
