phet commented on code in PR #3509:
URL: https://github.com/apache/gobblin/pull/3509#discussion_r879746911
##########
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:
I'm being super careful here (since gobblin users could somewhere, someday
even use an RDBMS that has yet to be invented!). if there were a bug in that
system, which made this query unworkable, they'll need ability to disable.
whereas the other uses of the validation query are specifically with mysql,
this is not necessarily so here. therefore I endorse keeping this relatively
inexpensive (and sensibly defaulting) config... better safe than sorry, right?
##########
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:
are you asking what would happen if the value used on line 79 is set to be
effectively infinite? I'm honestly not 100% sure, but suspect you're correct.
this is used by the troubleshooter/issues store. I likewise stopped to
ponder this comment and ultimately decided not to touch, because a) I have no
indication anything problematic has been happening and b) I consider the issues
store a P1 rather than P0 data tier (as e.g. state store and flow configs are).
that predisposed me less to question the optimization tradeoff described. do
you have any concerns to recommend pursuing at this time?
--
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]
