[go function] support localrun and cluster mode for go function #4174

wolfstudy · 2019-04-30T02:57:52Z

Signed-off-by: xiaolong.ran ranxiaolong716@gmail.com

Motivation

Master Issue: #3767

support local-run and cluster mode for go function.

in go function, we can use:

./bin/pulsar-admin functions localrun/create  
--go /Users/wolf4j/github.com/apache/pulsar/pulsar-function-go/examples/outputFunc.go 
--inputs persistent://public/default/my-topic 
--output persistent://public/default/test 
--tenant public 
--namespace default 
--name pulsarfunction 
--classname hellopulsar 
--log-topic logtopic

Different from --jar or --py, --go uploads a complete executable file(including: instance file + user code file)

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

sijie

@wolfstudy great move for supporting go function!

overall looks pretty good. left a few suggestions. PTAL.

It would be good to have an integration test for go function since now it supports running in cluster mode.

sijie · 2019-04-30T03:46:34Z

pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdFunctions.java

+        @Parameter(
+                names = "--go",
+                description = "Path to the main Go file for the function (if the function is written in Go)")
+        protected String goFile;


Nit: I think we are beginning having more and more branches in the pulsar runtime. I guess it might be the time to do a refactor to make the language runtime pluggable in Pulsar Function. So that we don't need to touch the runtime each time we introduce a new language. You don't need to do it right now. but It might be worth creating a task to track that.

sijie · 2019-04-30T03:47:45Z

pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdFunctions.java

        protected String pyFile;
+        @Parameter(
+                names = "--go",
+                description = "Path to the main Go file for the function (if the function is written in Go)")


I don't think it is the go file. It should be the built go program.

sijie · 2019-04-30T03:49:04Z

pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdFunctions.java

-            if (isNotBlank(functionConfig.getJar()) && isNotBlank(functionConfig.getPy())) {
-                throw new ParameterException("Either a Java jar or a Python file needs to"
+            if (isNotBlank(functionConfig.getJar()) && isNotBlank(functionConfig.getPy()) && isNotBlank(functionConfig.getGo())) {
+                throw new ParameterException("Either a Java jar or a Python file or a Go file needs to"


I would suggest using "Go executable binary" rather than "Go file". A "Go file" is a bit confusing, since it typically means the source files suffixed with .go.

yes, in here, we uploaded a source files suffixed with .go.

in getGoInstanceCmd()

args.add("go"); args.add("run"); args.add(originalCodeFileName);

We use go run to run this go file, go run can be divided into two steps:

build .go file is an executable binary

run this executable binary

In this way, the user only needs to upload the .go file, and it is not necessary to upload a compiled binary file.

@wolfstudy

Actually I forgot adding the comment there. I don't think we should use "go run", because not every machine installed go. so I would prefer uploading the executable binary.

awesome, will fix it

sijie · 2019-04-30T03:53:59Z

pulsar-function-go/conf/conf.go

 	Tenant               string `yaml:"tenant"`
 	NameSpace            string `yaml:"nameSpace"`
 	Name                 string `yaml:"name"`
+	ClassName            string `yaml:"className"`


I don't think Go needs ClassName.

Go has a completely different way to run functions than Java and Python.

In Java or Python, the instance code is isolated from user code since they support dynamically loading the code. So you need two parameters: --jar or --py to specify whether the function code; and --className to tell the function runtime to the entrypoint (aka className) to invoke the function.

However Go is a static-linking language. The instance is compiled with user function. There is only one entrypoint (which is the main func) for invoking the user function. Hence we only need the code file for go but we don't need a className. So I would suggest not adding a field if it is not used.

sijie · 2019-04-30T04:06:36Z

...src/main/java/org/apache/pulsar/functions/instance/functionforgo/FunctionConverGoConfig.java

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.functions.instance.functionforgo;


How about just calling this package "org.apache.pulsar.functions.instance.go"? That means it is the package related to go instance.

Excellent, go is a better choice than functionforgo, will fix it.

sijie · 2019-04-30T04:24:46Z

.../apache/pulsar/functions/secretsproviderconfigurator/DefaultSecretsProviderConfigurator.java

                return ClearTextSecretsProvider.class.getName();
            case PYTHON:
                return "secretsprovider.ClearTextSecretsProvider";
+            case GO:


throw new UnsupportedOperationException if a feature is not implemented.

throw new UnsupportedOperationException();

sijie · 2019-04-30T04:24:57Z

...ache/pulsar/functions/secretsproviderconfigurator/KubernetesSecretsProviderConfigurator.java

            case PYTHON:
                return "secretsprovider.EnvironmentBasedSecretsProvider";
+            case GO:
+                return "";


throw new UnsupportedOperationException if a feature is not implemented.

sijie · 2019-04-30T04:25:53Z

pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionConfigUtils.java

+        }
+
+        if (functionConfig.getWindowConfig() != null) {
+            throw new IllegalArgumentException("There is currently no support windowing in golang");


Suggested change

throw new IllegalArgumentException("There is currently no support windowing in golang");

throw new IllegalArgumentException("Windowing is not supported in Go function yet");

sijie · 2019-04-30T04:26:06Z

pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionConfigUtils.java


+    private static void doGolangChecks(FunctionConfig functionConfig) {
+        if (functionConfig.getProcessingGuarantees() == FunctionConfig.ProcessingGuarantees.EFFECTIVELY_ONCE) {
+            throw new RuntimeException("Effectively-once processing guarantees not yet supported in Golang");


Suggested change

throw new RuntimeException("Effectively-once processing guarantees not yet supported in Golang");

throw new RuntimeException("Effectively-once processing guarantees not yet supported in Go function");

sijie · 2019-04-30T04:26:23Z

pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionConfigUtils.java

+        }
+
+        if (functionConfig.getMaxMessageRetries() != null && functionConfig.getMaxMessageRetries() >= 0) {
+            throw new IllegalArgumentException("Message retries not yet supported in golang");


Suggested change

throw new IllegalArgumentException("Message retries not yet supported in golang");

throw new IllegalArgumentException("Message retries not yet supported in Go function");

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

jerrypeng · 2019-04-30T17:37:04Z

pom.xml

        <version>1.48</version>
      </dependency>

+      <dependency>


Same comment about using Jackson instead

jerrypeng · 2019-04-30T17:40:04Z

pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java

-import java.io.BufferedReader;
-import java.io.IOException;
-import java.io.InputStreamReader;
+import java.io.*;


Please don't use wildcards

cool, good suggestion

jerrypeng · 2019-04-30T17:52:17Z

pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/LocalRunner.java

                    classLoader = loadJar(file);
                }
+            } else if (functionConfig.getRuntime() == FunctionConfig.Runtime.GO) {
+                userCodeFile = functionConfig.getGo();


Can we also check for python runtime here and throw an exception if its not Java, Go, or python

jerrypeng · 2019-04-30T19:39:40Z

pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java

+        String output = yaml.dumpAs(goInstanceConfig, Tag.MAP, null);
+        String fileName = String.format("%s_%s_%s", goInstanceConfig.getTenant(), goInstanceConfig.getNameSpace(),
+                goInstanceConfig.getName());
+        File ymlFile = File.createTempFile(fileName, ".yml");


This file needs to be cleaned somehow.

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

wolfstudy · 2019-05-01T05:51:51Z

@jerrypeng PTAL again thanks

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

wolfstudy · 2019-05-02T07:00:21Z

@sijie @jerrypeng PTAL again

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

wolfstudy · 2019-05-02T13:23:54Z

run java8 tests

jerrypeng · 2019-05-02T17:46:33Z

pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java

+        goInstanceConfig.setKillAfterIdleMs(0);
+
+        // Parse the contents of goInstanceConfig into json form string
+        ObjectMapper objectMapper = new ObjectMapper(new JsonFactory());


please use ObjectMapperFactory.getThreadLocal() instead of creating a new ObjectMapper

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

sijie

overall looks great! left one mintor comment - UnsupportedOperationException should be thrown when a method is not implemented.

sijie · 2019-05-03T05:39:36Z

.../apache/pulsar/functions/secretsproviderconfigurator/DefaultSecretsProviderConfigurator.java

                return ClearTextSecretsProvider.class.getName();
            case PYTHON:
                return "secretsprovider.ClearTextSecretsProvider";
+            case GO:


throw new UnsupportedOperationException();

sijie · 2019-05-03T05:41:07Z

run java8 tests

jerrypeng · 2019-05-03T06:24:30Z

pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java

            if (StringUtils.isNotEmpty(extraDependenciesDir)) {
                args.add("PYTHONPATH=${PYTHONPATH}:" + extraDependenciesDir);
            }
+        } else if (instanceConfig.getFunctionDetails().getRuntime() == Function.FunctionDetails.Runtime.GO) {


I don't think this check is necessary

jerrypeng · 2019-05-03T06:26:58Z

LGTM @srkukarni you want to take a look as well?

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

wolfstudy · 2019-05-04T05:36:09Z

run java8 tests

wolfstudy · 2019-05-04T08:18:05Z

run java8 tests

wolfstudy · 2019-05-04T14:43:07Z

ping @srkukarni
do you want to take a look as well?

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

[go function] support localrun and cluster mode for go function

1259573

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

sijie requested changes Apr 30, 2019

View reviewed changes

sijie assigned wolfstudy Apr 30, 2019

sijie added area/function component/go labels Apr 30, 2019

sijie added this to the 2.4.0 milestone Apr 30, 2019

sijie added the type/feature The PR added a new feature or issue requested a new feature label Apr 30, 2019

fix comments

a256eb9

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

wolfstudy mentioned this pull request Apr 30, 2019

[pulsar function] refactor function runtime #4176

Closed

jerrypeng reviewed Apr 30, 2019

View reviewed changes

pom.xml Outdated

<version>1.48</version>

</dependency>

<dependency>

Copy link

Contributor

jerrypeng Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about using Jackson instead

jerrypeng reviewed Apr 30, 2019

View reviewed changes

wolfstudy added 2 commits May 1, 2019 12:23

fix some comments

7dbbe04

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

use jackson instead of snakeyaml

930702f

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

wolfstudy added 2 commits May 2, 2019 11:09

use conf content instead of conf file path

e46c9ea

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

fix java unit tests

aa0282e

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

fix java test(Function classname cannot be null)

222bd2d

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

jerrypeng reviewed May 2, 2019

View reviewed changes

use ObjectMapperFactory.getThreadLocal()

701c72d

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

sijie approved these changes May 3, 2019

View reviewed changes

jerrypeng reviewed May 3, 2019

View reviewed changes

fix a little

c9dd188

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

fix logtopic error logic

2fcb281

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>

sijie merged commit 42c3bf9 into apache:master May 6, 2019

sijie mentioned this pull request Dec 27, 2019

ISSUE-4176: [pulsar function] refactor function runtime streamnative/pulsar-archived#489

Closed

	throw new IllegalArgumentException("There is currently no support windowing in golang");
	throw new IllegalArgumentException("Windowing is not supported in Go function yet");

	throw new RuntimeException("Effectively-once processing guarantees not yet supported in Golang");
	throw new RuntimeException("Effectively-once processing guarantees not yet supported in Go function");

	throw new IllegalArgumentException("Message retries not yet supported in golang");
	throw new IllegalArgumentException("Message retries not yet supported in Go function");

[go function] support localrun and cluster mode for go function #4174

[go function] support localrun and cluster mode for go function #4174

Uh oh!

Conversation

wolfstudy commented Apr 30, 2019 • edited by sijie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfstudy Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfstudy Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfstudy commented May 1, 2019

Uh oh!

wolfstudy commented May 2, 2019

Uh oh!

wolfstudy commented May 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sijie commented May 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerrypeng commented May 3, 2019

Uh oh!

wolfstudy commented May 4, 2019

Uh oh!

wolfstudy commented May 4, 2019

Uh oh!

wolfstudy commented May 4, 2019

Uh oh!

wolfstudy commented Apr 30, 2019 •

edited by sijie

Loading

wolfstudy Apr 30, 2019 •

edited

Loading

wolfstudy Apr 30, 2019 •

edited

Loading