[GitHub] [pulsar] jerrypeng commented on a change in pull request #4174: [go function] support localrun and cluster mode for go function

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

jerrypeng commented on a change in pull request #4174: [go function] support localrun and cluster mode for go function
URL: https://github.com/apache/pulsar/pull/4174#discussion_r280004106
 
 

 ##########
 File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
 ##########
 @@ -89,31 +91,180 @@
             if (StringUtils.isNotEmpty(extraDependenciesDir)) {
                 args.add("PYTHONPATH=${PYTHONPATH}:" + extraDependenciesDir);
             }
+        } else if (instanceConfig.getFunctionDetails().getRuntime() == Function.FunctionDetails.Runtime.GO) {
+            //no-op
         }
 
         return args;
     }
 
+    /**
+     *
+     * Different from python and java function, Go function uploads a complete executable file(including:
+     * instance file + user code file). Its parameter list is provided to the broker in the form of a yaml file,
+     * the advantage of this approach is that backward compatibility is guaranteed.
 
 Review comment:
   I think there is a config you can set to to ignore unknown flags. Please reference https://godoc.org/github.com/jessevdk/go-flags.
   
   ```
   Ignoring unknown command line options (optional)
   ```
   
   Even if are using files to passing arguments to the GO instance why does it have to be the arguments in instance config?  Why not just write the command line arguments in a file to begin with?  Given we can ignore unknown flags in GO, then BC i.e. adding new flags shouldn't be a problem.
   
   > When talking about BC, it is not about what current is. It is about what the future will be. We are not using positional arguments for now. but will it be used in future?
   
   I don't think we will ever add positional arguments because that will become a BC issue for all the runtimes.
   
   > At least, it was not a fun to me when debugging any instance related issues for Java and Python. Because a) the command line is just too long; b) you have to get all function details encoded to a string that is able to be passed in a command line; c) the encoded function details is almost not human-readable.
   
   These are superficial problems right. A user can also copy and paste the command line into a text editor and then parse it however he/she would like.  
   
   However, using a file to pass arguments to function instances will create fundamental challenges we will have to solve.  Sure for K8s, we can maybe use a configmap and mount it to pods, but in the future what about other runtimes.  For example, if we wanted to support just a docker runtime how would we do that.  We would maybe need to mount a directory from local disk to the container.  What about if we want to support another external scheduler like Nomad or Mesos, it has it own way of mapping files into containers.  My point is adding this file dependency will create more moving parts and leave more room for error and, as far as, I can tell every scheduler allows your to specify command line arguments to run a process or container but placing files to where your container/process will run can be widely different.  On top of that we will also have to figure out how to clean up these files for non-container based runtimes.
   
   While all of this is not impossible to do, my question is it really worth it?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services