Support running function worker along with broker #1329

sijie · 2018-03-05T05:54:51Z

Motivation

It would make things easier to support run function worker along with broker. For example, user can turn on pulsar-functions by just adding a new flag in command line: e.g. bin/pulsar standalone --run-function-worker.

Modifications

The major challenge of running function worker along with broker is to avoid conflicting on protobuf version. So the main idea of this change:

have a pulsar-function-worker-shaded module shade protobuf
pulsar-broker includes pulsar-function-worker-shaded: start a worker service when --run-function-worker is specified, registered functions rest endpoints under admin/functions and admin/v2/functions.
update conf/log4j2.yml and bin/pulsar for adopting function worker in pulsar startup scripts.

The details of the change:

within pulsar-functions

add pulsar-function-worker-shaded: it shades protobuf3 and includes grpc and the dependencies that uses protobuf3.
add pulsar-function-worker-runner, which exclude pulsar-client-tools-shaded and includes the original client/admin module. It is acting as a testing module to make sure pulsar-function-worker-shaded can work with pulsar-broker.
points the existing scripts to use the pulsar-function-worker-runner. The existing functions scripts remain here util we merge function cli into pulsar-tools

pulsar-broker

move tests from pulsar-client-tools into pulsar-client-tools-test. because pulsar-client-tools depends on pulsar-broker on testing. this would introduce recursive dependency when we attempt to include pulsar-function-worker-shaded in pulsar-broker.
update PulsarBrokerStarter and PulsarStandaloneStarter to start function worker along with it.

script

include function_worker.yml in conf directory
update conf/log4j2.yml to include RoutingAppender, so broker can use it for routing functions' logging.
update bin/pulsar to find java/python instances

Result

we are able to start function-worker with bin/pulsar standalone --run-function-worker.

NOTES:

this change doesn't touch the CLI part.
this change doesn't touch the distribution part. i

…cated license headers This addresses some comments in pulsar functions PR apache#1314

…mments

…keeper version This removes the temp dependency in `pulsar-functions-instance`

sijie · 2018-03-05T05:55:38Z

/cc @srkukarni @jerrypeng

merlimat · 2018-03-05T05:58:59Z

I think for standalone service, this should be enabled by default.

srkukarni · 2018-03-05T06:04:37Z

agree with @merlimat

jerrypeng · 2018-03-05T06:09:33Z

Yup

merlimat · 2018-03-05T06:01:16Z

bin/pulsar

@@ -74,6 +110,7 @@ where command is one of:
    discovery           Run a discovery server
    proxy               Run a pulsar proxy
    websocket           Run a web socket proxy server
+    worker              Run a function worker server


what about naming the command functions-worker to make it more explicit? Also, use plural to highlight this will be running multiple instances of multiple functions.

merlimat · 2018-03-05T06:02:30Z

buildtools/pom.xml

@@ -40,6 +40,21 @@
      <artifactId>testng</artifactId>
      <version>6.13.1</version>
    </dependency>
+    <dependency>


If every module depends on buildtool, everyone will depend on log4j

the log4j are already default to all modules in test scope, buildtools is also default to all modules in test scope, since it is shipping a default log4j2.yaml for testing. so it is making sense to move them to buildtools.

merlimat · 2018-03-05T06:06:01Z

conf/function_worker.yml

+# under the License.
+#
+
+workerId: standalone


We should have comments for each key explaining the purpose of each setting.

Ideally, each setting should also have the default value, such that commenting that same line would have the same effect.

this file is currently copied from functions-module, which I don't change any behavior at this moment. I would keep this change as simple as for now since it is already touching quite a few places. we can clean up these configuration files after we merge the functions dist with pulsar/all.

merlimat · 2018-03-05T06:06:53Z

conf/function_worker.yml

+pulsarServiceUrl: pulsar://localhost:6650
+pulsarWebServiceUrl: http://localhost:8080
+numFunctionPackageReplicas: 1
+downloadDirectory: /tmp/pulsar_functions


we should default to data/pulsar_functions. /tmp is mounted on ramdisk in many systems.

I think the purpose of using /tmp because worker doesn't really require any persistence. the directory can be gone after reboot, that's okay. but we can change it to data/ if that's preferred. although it would be good to do this after this change when we are merging function dist with pulsar/all. the file is copied from functions/conf to conf.

merlimat · 2018-03-05T06:08:43Z

conf/function_worker.yml

+metricsConfig:
+  metricsSinkClassName: org.apache.pulsar.functions.metrics.sink.PrometheusSink
+  metricsCollectionInterval: 30
+  metricsSinkConfig:


If we collect the metrics with the Prometheus java client lib, the metrics are already in the registry and will be reported by standalone broker at localhost:8080/metrics

I think this is for process based runtime. @srkukarni can chime in here.

@sijie This is for both process and thread runtimes. @merlimat I was hoping to integrate with Pulsar's inbuilt metrics after the merge.

merlimat · 2018-03-05T06:12:16Z

pulsar-broker/src/main/java/org/apache/pulsar/PulsarStandaloneStarter.java

@@ -76,6 +81,12 @@
    @Parameter(names = { "--only-broker" }, description = "Only start Pulsar broker service (no ZK, BK)")
    private boolean onlyBroker = false;

+    @Parameter(names = {"-rfw", "--run-function-worker"}, description = "Run function worker with Broker")


--run-functions-worker

Also, we should have a key in broker.conf to do the same

will add one.

jerrypeng · 2018-03-05T06:13:25Z

@sijie In the worker we are issue calls the Pulsar broker's admin REST interface. In the future, for better integration, can the pulsar broker pass in objects to the worker so it can use that determine the info gotten from the admin REST interface instead of actually issuing a HTTP command locally?

sijie · 2018-03-05T06:34:14Z

In the worker we are issue calls the Pulsar broker's admin REST interface. In the future, for better integration, can the pulsar broker pass in objects to the worker so it can use that determine the info gotten from the admin REST interface instead of actually issuing a HTTP command locally?

@jerrypeng yes. that would be the ideal solution. however that is going to take more efforts to achieve that, which should be done after we make function worker integrated on broker.

sijie · 2018-03-05T06:36:36Z

for standalone service, this should be enabled by default.

@jerrypeng @srkukarni @merlimat : the console output of standalone is already very complicated, making standalone by default starts function worker will make it worse. are we sure we want to make it by default.

If we think start function worker should be the default behavior, I can make the change.

srkukarni · 2018-03-05T06:41:47Z

@sijie I still think that we should make it default for standalone. Users will be typically more interested in actual function logs(either in log files or log topic) than the console output of broker.

merlimat · 2018-03-05T06:44:39Z

@jerrypeng @srkukarni @merlimat : the console output of standalone is already very complicated, making standalone by default starts function worker will make it worse. are we sure we want to make it by default.

Would it be feasible to route all logs to a separate file?

sijie · 2018-03-05T06:47:00Z

@merlimat we can route based on classes org.apache.pulsar.functions.

@srkukarni okay will make it default.

sijie · 2018-03-05T07:29:59Z

@merlimat @srkukarni @jerrypeng :

addressed your comments. make functions worker started by default in standalone, it can be turned off by "--no-functions-worker".

@merlimat :

changed "function worker" to "functions worker" and also add a setting in server configuration for broker.

merlimat

👍

srkukarni · 2018-03-05T21:52:38Z

LGTM!

sijie added 13 commits March 4, 2018 02:21

Move pulsar functions dependency version to root pom and remove dupli…

641df06

…cated license headers This addresses some comments in pulsar functions PR apache#1314

shade worker

ef65a9f

Merge remote-tracking branch 'apache/master' into address_function_co…

11e82f3

…mments

Fix broken master

7bc05f1

Upgrade the bookkeeper storage client dependency to the official book…

fc8d4c5

…keeper version This removes the temp dependency in `pulsar-functions-instance`

Merge branch 'upgrade_bk_dependencies' into shade_worker

07cb9d2

set protobuf2.version in pulsar-common

6b494c6

Merge branch 'address_function_comments' into shade_worker

0744dbc

provide a shaded worker

59b10aa

include worker dependency at broker

0935c46

Merge remote-tracking branch 'apache/master' into shade_worker

579ba30

Embeded function worker at broker

e7daf2d

Merge remote-tracking branch 'apache/master' into shade_worker

f8bde86

sijie self-assigned this Mar 5, 2018

sijie requested review from merlimat and rdhabalia March 5, 2018 05:54

merlimat reviewed Mar 5, 2018

View reviewed changes

rename 'function worker' to 'functions worker'

f30a7c2

sijie mentioned this pull request Mar 5, 2018

Integrate function cli into pulsar-admin cli #1331

Merged

add "--no-functions-worker" for pulsar-client-cpp tests

ce20823

merlimat approved these changes Mar 5, 2018

View reviewed changes

merlimat added this to the 2.0.0-incubating milestone Mar 5, 2018

merlimat merged commit b701925 into apache:master Mar 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support running function worker along with broker #1329

Support running function worker along with broker #1329

sijie commented Mar 5, 2018

sijie commented Mar 5, 2018

merlimat commented Mar 5, 2018

srkukarni commented Mar 5, 2018

jerrypeng commented Mar 5, 2018

merlimat Mar 5, 2018

merlimat Mar 5, 2018

sijie Mar 5, 2018

merlimat Mar 5, 2018

sijie Mar 5, 2018

merlimat Mar 5, 2018

sijie Mar 5, 2018

merlimat Mar 5, 2018

sijie Mar 5, 2018

srkukarni Mar 5, 2018

merlimat Mar 5, 2018

sijie Mar 5, 2018

jerrypeng commented Mar 5, 2018

sijie commented Mar 5, 2018

sijie commented Mar 5, 2018

srkukarni commented Mar 5, 2018

merlimat commented Mar 5, 2018

sijie commented Mar 5, 2018

sijie commented Mar 5, 2018

merlimat left a comment

srkukarni commented Mar 5, 2018

Support running function worker along with broker #1329

Support running function worker along with broker #1329

Conversation

sijie commented Mar 5, 2018

Motivation

Modifications

Result

sijie commented Mar 5, 2018

merlimat commented Mar 5, 2018

srkukarni commented Mar 5, 2018

jerrypeng commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrypeng commented Mar 5, 2018

sijie commented Mar 5, 2018

sijie commented Mar 5, 2018

srkukarni commented Mar 5, 2018

merlimat commented Mar 5, 2018

sijie commented Mar 5, 2018

sijie commented Mar 5, 2018

merlimat left a comment

Choose a reason for hiding this comment

srkukarni commented Mar 5, 2018