[GitHub] [beam] chamikaramj commented on a diff in pull request #25824: Add driverJarDirectory parameter to JdbcIO.

["chamikaramj (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org