[GitHub] [beam] chamikaramj commented on a change in pull request #15857: Allow wildcards for java class lookup transform providers.

[GitBox <gi...@apache.org>]

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

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