chamikaramj commented on a change in pull request #15857:
URL: https://github.com/apache/beam/pull/15857#discussion_r743979210
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -283,12 +284,27 @@ def _has_constructor(self):
class JavaExternalTransform(ptransform.PTransform):
"""A proxy for Java-implemented external transforms.
- One builds these transforms just as one would in Java.
+ One builds these transforms just as one would in Java, e.g.::
+
+ transform = JavaExternalTransform('fully.qualified.ClassName'
+ )(contructorArg, ... ).builderMethod(...)
+
+ or::
+
+ JavaExternalTransform('fully.qualified.ClassName').staticConstructor(
+ ...).builderMethod1(...).builderMethod2(...)
+
+ :param class_name: fully qualified name of the java class
+ :param expansion_service: (Optional) an expansion service to use. If none is
+ provided, a default expansion service will be started.
+ :param classpath: (Optional) A list paths to additional jars to place on the
+ expansion service classpath.
"""
- def __init__(self, class_name, expansion_service=None):
+ def __init__(self, class_name, expansion_service=None, classpath=None):
+ assert not (expansion_service and classpath), (expansion_service,
classpath)
Review comment:
Did you expect to include an error message instead of
"(expansion_service, classpath)" ?
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -302,7 +318,11 @@ def __getattr__(self, name):
# Don't try to emulate special methods.
if name.startswith('__') and name.endswith('__'):
return super().__getattr__(name)
+ else:
+ return self[name]
+ def __getitem__(self, name):
Review comment:
What does moving this to a dictionary fix ? (for my knowledge)
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -655,22 +680,29 @@ class JavaJarExpansionService(object):
argument which will spawn a subprocess using this jar to expand the
transform.
"""
- def __init__(self, path_to_jar, extra_args=None):
- if extra_args is None:
Review comment:
Probably need to rebase again (based on the diff).
##########
File path: sdks/python/apache_beam/transforms/external_test.py
##########
@@ -547,6 +550,27 @@ def test_implicit_builder_with_constructor_method(self):
self._verify_row(build_method.schema, build_method.payload, {})
+class JavaJarExpansionServiceTest(unittest.TestCase):
+ def test_classpath(self):
Review comment:
BTW I think we should add more comprehensive unit testing for
"JavaJarExpansionService" and "BeamJarExpansionService" (can be a separate PR).
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -655,22 +680,29 @@ class JavaJarExpansionService(object):
argument which will spawn a subprocess using this jar to expand the
transform.
"""
- def __init__(self, path_to_jar, extra_args=None):
- if extra_args is None:
- extra_args = ['{{PORT}}', f'--filesToStage={path_to_jar}']
+ 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._service_count = 0
+ def _default_args(self):
+ to_stage = ','.join([self._path_to_jar] + sum(
Review comment:
Fail here if self._path_to_jar is not set ?
##########
File path: sdks/python/apache_beam/utils/subprocess_server.py
##########
@@ -158,9 +158,14 @@ class JavaJarServer(SubprocessServer):
'local', (threading.local, ),
dict(__init__=lambda self: setattr(self, 'replacements', {})))()
- def __init__(self, stub_class, path_to_jar, java_arguments):
+ def __init__(self, stub_class, path_to_jar, java_arguments, classpath=None):
+ if classpath:
+ cp_args = ['-classpath', os.pathsep.join(classpath)]
+ else:
+ cp_args = []
super().__init__(
- stub_class, ['java', '-jar', path_to_jar] + list(java_arguments))
+ stub_class,
+ ['java'] + cp_args + ['-jar', path_to_jar] + list(java_arguments))
Review comment:
Unfortunately I don't think "java -jar" command can take a CLASSPATH as
an argument. So this probably won't work.
https://docs.oracle.com/en/java/javase/17/docs/specs/man/jar.html
Im fine with partially rolling this back to use a single jar when we
automatically startup the expansion service.
--
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]
