chamikaramj commented on code in PR #25824:
URL: https://github.com/apache/beam/pull/25824#discussion_r1153483905
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java:
##########
@@ -583,17 +587,35 @@ public DataSourceConfiguration
withDriverClassLoader(ClassLoader driverClassLoad
return builder().setDriverClassLoader(driverClassLoader).build();
}
+ /**
+ * Comma separated Cloud Storage paths for JDBC drivers. If not specified,
the default class
+ * loader is used to load the jars.
Review Comment:
Seems like this will work for any FileSystem supported by Beam, not just GCS
?
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcSchemaIOProvider.java:
##########
@@ -209,6 +209,14 @@ protected JdbcIO.DataSourceConfiguration
getDataSourceConfiguration() {
}
}
+ if (config.getSchema().hasField("driverJars")) {
Review Comment:
Can you also please support the x-lang wrapper to support this ?
https://github.com/apache/beam/blob/master/sdks/python/apache_beam/io/jdbc.py
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcUtil.java:
##########
@@ -61,6 +73,42 @@
/** Provides utility functions for working with {@link JdbcIO}. */
class JdbcUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(JdbcUtil.class);
+
+ /** Utility method to save jar files locally in the worker. */
+ static URL[] saveFilesLocally(String driverJars) {
Review Comment:
Please add a unit test for this.
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/SchemaUtil.java:
##########
@@ -67,7 +67,7 @@
/** Provides utility functions for working with Beam {@link Schema} types. */
@Experimental(Kind.SCHEMAS)
-class SchemaUtil {
+public class SchemaUtil {
Review Comment:
If you want to make this public can you please add appropriate unit tests.
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java:
##########
@@ -583,17 +587,35 @@ public DataSourceConfiguration
withDriverClassLoader(ClassLoader driverClassLoad
return builder().setDriverClassLoader(driverClassLoader).build();
}
+ /**
+ * Comma separated Cloud Storage paths for JDBC drivers. If not specified,
the default class
+ * loader is used to load the jars.
+ *
+ * For example,
gs://your-bucket/driver_jar1.jar,gs://your-bucket/driver_jar2.jar.
Review Comment:
I think we should also add an end-to-end integration test for this feature.
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java:
##########
@@ -630,6 +652,11 @@ && getConnectionInitSqls().get() != null
if (getDriverClassLoader() != null) {
basicDataSource.setDriverClassLoader(getDriverClassLoader());
}
+ if (getDriverJars() != null) {
+ URLClassLoader classLoader =
Review Comment:
How will these jars get staged for runtime ?
--
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]
