chamikaramj commented on a change in pull request #16613:
URL: https://github.com/apache/beam/pull/16613#discussion_r797981414
##########
File path: sdks/python/apache_beam/io/jdbc.py
##########
@@ -181,8 +192,17 @@ def __init__(
:param connection_init_sqls: required only for MySql and MariaDB.
passed as list of strings
:param expansion_service: The address (host:port) of the ExpansionService.
+ :param classpath: A list of JARs or Java packages to include in the
+ classpath for the expansion service. This option is
+ usually needed for `jdbc` to include extra JDBC driver
+ packages.
+ The packages can be in these three formats: (1) A local
Review comment:
Let's make sure we have tests for these three cases.
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -694,12 +695,31 @@ class JavaJarExpansionService(object):
def __init__(self, path_to_jar, extra_args=None, classpath=None):
self._path_to_jar = path_to_jar
self._extra_args = extra_args
- self._classpath = classpath
+ self._classpath = classpath or []
self._service_count = 0
+ @staticmethod
+ def _expand_jars(jar):
+ if glob.glob(jar):
+ return glob.glob(jar)
+ elif isinstance(jar, str) and (jar.startswith('http://') or
+ jar.startswith('https://')):
+ return [jar]
+ else:
+ try:
Review comment:
Please add a comment to explain the "else" clause here.
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -694,12 +695,31 @@ class JavaJarExpansionService(object):
def __init__(self, path_to_jar, extra_args=None, classpath=None):
self._path_to_jar = path_to_jar
self._extra_args = extra_args
- self._classpath = classpath
+ self._classpath = classpath or []
self._service_count = 0
+ @staticmethod
+ def _expand_jars(jar):
+ if glob.glob(jar):
+ return glob.glob(jar)
+ elif isinstance(jar, str) and (jar.startswith('http://') or
+ jar.startswith('https://')):
+ return [jar]
+ else:
+ try:
+ group_id, artifact_id, version = jar.split(':')
+ except ValueError:
+ # If we are not able to
+ logging.warning('Unable to parse %s into group:artifact:version.', jar)
+ return [jar]
Review comment:
Do you expect a local jar as a string here ? If so this should be in the
regular path not the exception path. In the exception path we should just fail.
##########
File path: sdks/python/apache_beam/io/jdbc.py
##########
@@ -119,6 +127,8 @@ def default_io_expansion_service():
],
)
+DEFAULT_JDBC_CLASSPATH = ['org.postgresql:postgresql:42.2.16']
Review comment:
Noting that this will not be captured by our regular dependency upgrade
notifications but this might be OK.
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -694,12 +695,31 @@ class JavaJarExpansionService(object):
def __init__(self, path_to_jar, extra_args=None, classpath=None):
self._path_to_jar = path_to_jar
self._extra_args = extra_args
- self._classpath = classpath
+ self._classpath = classpath or []
self._service_count = 0
+ @staticmethod
+ def _expand_jars(jar):
+ if glob.glob(jar):
+ return glob.glob(jar)
+ elif isinstance(jar, str) and (jar.startswith('http://') or
+ jar.startswith('https://')):
+ return [jar]
+ else:
+ try:
+ group_id, artifact_id, version = jar.split(':')
+ except ValueError:
+ # If we are not able to
Review comment:
Incomplete comment.
##########
File path: sdks/python/setup.py
##########
@@ -175,7 +175,8 @@ def get_version():
'pytest-timeout>=1.3.3,<2',
'sqlalchemy>=1.3,<2.0',
'psycopg2-binary>=2.8.5,<3.0.0',
- 'testcontainers>=3.0.3,<4.0.0',
+ 'testcontainers[mysql]>=3.0.3,<4.0.0',
+ 'cryptography>=36.0.0',
Review comment:
Why was this needed ?
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -709,11 +729,21 @@ def __enter__(self):
if self._extra_args is None:
self._extra_args = self._default_args()
# Consider memoizing these servers (with some timeout).
+ logging.info(
+ 'Starting a JAR-based expansion service from JAR %s and '
+ 'with classpath: %s',
Review comment:
Probably only log the "with classpath ..." part if classpath is not
empty.
--
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]
