chamikaramj commented on code in PR #25824:
URL: https://github.com/apache/beam/pull/25824#discussion_r1144103211
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java:
##########
@@ -627,8 +644,32 @@ && getConnectionInitSqls().get() != null
if (getMaxConnections() != null && getMaxConnections().get() != null) {
basicDataSource.setMaxTotal(getMaxConnections().get());
}
- if (getDriverClassLoader() != null) {
- basicDataSource.setDriverClassLoader(getDriverClassLoader());
+ if (getDriverJarDirectory() != null && getDriverJarDirectory().get()
!= null) {
+ try {
+ String pathPrefix =
+ getDriverJarDirectory().get().endsWith("/")
Review Comment:
Hardcoding "/" will break this for windows. Please use
System.lineSeparator() instead. Also please make this a util instead of
directly including the code in the I/O.
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java:
##########
@@ -575,12 +583,21 @@ public DataSourceConfiguration withMaxConnections(
}
/**
- * Sets the class loader instance to be used to load the JDBC driver. If
not specified, the
- * default class loader is used.
+ * Sets the directory to the JDBC driver in the worker. If specified, the
path is used to load
+ * the driver jar. If not specified, the default class loader is used to
load the jars.
+ *
+ * <p>NOTE - driverJarDirectory must be the absolute path to the directory.
*/
- public DataSourceConfiguration withDriverClassLoader(ClassLoader
driverClassLoader) {
- checkArgument(driverClassLoader != null, "driverClassLoader can not be
null");
- return builder().setDriverClassLoader(driverClassLoader).build();
+ public DataSourceConfiguration withDriverJarDirectory(String
driverJarDirectory) {
+ checkArgument(driverJarDirectory != null, "driverJarDirectory can not be
null");
+ return
withDriverJarDirectory(ValueProvider.StaticValueProvider.of(driverJarDirectory));
+ }
+
+ /** Same as {@link #withDriverJarDirectory(String)} but accepting a
ValueProvider. */
+ public DataSourceConfiguration withDriverJarDirectory(
+ ValueProvider<@Nullable String> driverJarDirectory) {
Review Comment:
I believe, ValueProviders are not needed with Flex templates, right ?
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java:
##########
@@ -627,8 +644,32 @@ && getConnectionInitSqls().get() != null
if (getMaxConnections() != null && getMaxConnections().get() != null) {
basicDataSource.setMaxTotal(getMaxConnections().get());
}
- if (getDriverClassLoader() != null) {
- basicDataSource.setDriverClassLoader(getDriverClassLoader());
+ if (getDriverJarDirectory() != null && getDriverJarDirectory().get()
!= null) {
+ try {
+ String pathPrefix =
+ getDriverJarDirectory().get().endsWith("/")
+ ? getDriverJarDirectory().get()
+ : getDriverJarDirectory().get() + "/";
+ MatchResult result = FileSystems.match(pathPrefix + "**",
EmptyMatchTreatment.ALLOW);
Review Comment:
I'm not sure if the FileSystems API will work correctly at job submission
without initialization (it's intended to be primarily used at workers). Did we
test using GCS etc. ?
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java:
##########
@@ -449,7 +457,7 @@ public abstract static class DataSourceConfiguration
implements Serializable {
abstract @Nullable ValueProvider<Integer> getMaxConnections();
@Pure
- abstract @Nullable ClassLoader getDriverClassLoader();
+ abstract @Nullable ValueProvider<String> getDriverJarDirectory();
Review Comment:
We cannot remove this getDriverClassLoader() due to backwards compatibility.
Can we just add the new method instead.
--
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]
