chamikaramj commented on a change in pull request #15857:
URL: https://github.com/apache/beam/pull/15857#discussion_r741540490
##########
File path:
sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java
##########
@@ -508,12 +523,32 @@ static AllowList create(
@AutoValue
public abstract static class AllowedClass {
+ public static final List<String> WILDCARD = Collections.singletonList("*");
+
public abstract String getClassName();
public abstract List<String> getAllowedBuilderMethods();
public abstract List<String> getAllowedConstructorMethods();
+ public boolean isAllowedClass(String className) {
+ String pattern = getClassName();
Review comment:
Probably we should document.
* How to create an allowlist file to allow everything.
* How to create an allowlist file to just allow a given package.
##########
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):
Review comment:
Could we add a unit test to cover this ?
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -283,9 +283,21 @@ 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')(
+ contrustorArg, ...).builderMethod(...)
+
+ or::
+
+ JavaExternalTransform('fully.qualified.ClassName').staticConstructor(
+ ...).builderMethod1(...).builderMethod2(...)
"""
- def __init__(self, class_name, expansion_service=None):
+ def __init__(self, class_name, expansion_service=None, classpath=None):
+ expansion_service = expansion_service or BeamJarExpansionService(
Review comment:
So the default version will just offer the classes packaged with the
<expansion-service> package ? I'm not sure whether this will be useful (it's
whatever in that package and Beam core). Should we make providing classpath (or
additional jars) required to make the default usage useful ?
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -283,9 +283,21 @@ 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')(
+ contrustorArg, ...).builderMethod(...)
+
+ or::
+
+ JavaExternalTransform('fully.qualified.ClassName').staticConstructor(
Review comment:
JavaExternalTransform('fully.qualified.ClassName')(\<params\>) works as
well, right ?
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -283,9 +283,21 @@ 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')(
+ contrustorArg, ...).builderMethod(...)
+
+ or::
+
+ JavaExternalTransform('fully.qualified.ClassName').staticConstructor(
+ ...).builderMethod1(...).builderMethod2(...)
"""
- def __init__(self, class_name, expansion_service=None):
+ def __init__(self, class_name, expansion_service=None, classpath=None):
Review comment:
Please document what classpath should be (seems like a list based on the
usage below).
##########
File path:
sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java
##########
@@ -53,6 +52,9 @@ public AllowList create(PipelineOptions options) {
String allowListFile =
options.as(ExpansionServiceOptions.class).getJavaClassLookupAllowlistFile();
if (allowListFile != null) {
+ if (allowListFile.equals("*")) {
Review comment:
Please document the updated usage in the corresponding property of
ExpansionServiceOptions.
##########
File path: sdks/python/apache_beam/transforms/external.py
##########
@@ -655,11 +667,13 @@ 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):
+ def __init__(self, path_to_jar, extra_args=None, classpath=None):
if extra_args is None:
- extra_args = ['{{PORT}}', f'--filesToStage={path_to_jar}']
+ to_stage = ','.join([path_to_jar] + list(classpath or []))
Review comment:
Seems like we assume that the provided classpath only contains jars ?
Classpath also supports directories and wild cards.
https://docs.oracle.com/javase/8/docs/technotes/tools/windows/classpath.html
--
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]
