[fix #8089] expose PulsarAdmin client through Function Context #9246

freeznet · 2021-01-20T12:46:38Z

Motivation

#8089 addressed that making pulsar admin client api calls is conflict with Jersey library. This PR expose PulsarAdmin client through Function Context, so the function implementers can use PulsarAdmin to do whatever they want to do.

Modifications

add getPulsarAdmin() and getPulsarAdmin(String clusterName) in function Context
add implementation of above interface in ContextImpl
add passing pulsarWebServiceUrl to function runtimes
add passing pulsarWebServiceUrl to LocalRunner / JavaInstanceStarter
add "--web-service-url" parameter to LocalRunner admin command
add unit tests
add example function
add exposeAdminClientEnabled in functions_worker.yml

Verifying this change

Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

add unit tests to verify the args passing from process runtime / k8s runtime to JavaInstanceStarter
add unit tests to verify getPulsarAdmin
add integration tests to verify if pulsar admin works well [WIP]

Does this pull request potentially affect one of the following parts:

The admin cli options: (yes)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

eolivelli · 2021-01-20T13:13:33Z

pulsar-functions/api-java/src/main/java/org/apache/pulsar/functions/api/Context.java

+    PulsarAdmin getPulsarAdmin();
+
+    /**
+     * Get the pulsar admin client by cluster name.


can we explain here when it is better to use this method instead of the other ?

Pulsar recently adds support for multiple clusters, i.e. input topics from cluster A and output topics from cluster B. This method just provides the same support for admin calls.

yes, just as sijie said, getPulsarAdmin() will just get the default pulsar admin client, the other one will let user get pulsar admin client if externalPulsarClusters is set.

pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/instance/ContextImpl.java

sijie · 2021-01-20T20:59:58Z

@nlu90 Can you review this since you add the multiple-cluster support there?

sijie

@freeznet the overall change looks really strong! I left a couple of comments regarding backward compatibility. Please take a look.

pulsar-functions/api-java/pom.xml

pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/instance/InstanceUtils.java

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

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

...functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/JavaInstanceStarter.java

sijie · 2021-01-20T21:53:39Z

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

+        if (instanceConfig.getFunctionDetails().getRuntime() == Function.FunctionDetails.Runtime.JAVA) {
+            // TODO: for now only Java function context exposed pulsar admin, so python/go no need to pass this argument
+            // until pulsar admin client enabled in python/go function context.
+            if (pulsarWebServiceUrl != null) {


we need to add a flag in the function worker config to control whether to expose PulsarAdmin through context. Because the new function worker will add this --web_serviceurl to the command line that is not able to recognize by the function instance. So we need to have a flag to allow people to do the following update sequence:

Update function worker with exposing pulsar admin disabled.

Update all the functions to the new image that understand --web_serviceurl

Update function worker to enable exposing pulsar admin.

I agree, flag added, also add some tests when exposePulsarAdminClientEnabled=false

freeznet · 2021-01-25T13:42:50Z

@eolivelli @sijie @nlu90 code changed according to your review, PTAL when you have time, thanks

sijie

@freeznet The change looks really great! Thank you for your contribution!

eolivelli

Lgtm

Thanks for this feature.
It will ease developing functions and especially connectors

freeznet · 2021-01-28T00:59:38Z

/pulsarbot run-failure-checks

freeznet · 2021-01-31T04:35:49Z

/pulsarbot run-failure-checks

freeznet · 2021-01-31T12:10:05Z

/pulsarbot run-failure-checks

freeznet · 2021-01-31T14:49:38Z

/pulsarbot run-failure-checks

freeznet · 2021-02-01T06:25:58Z

/pulsarbot run-failure-checks

freeznet · 2021-02-01T11:05:30Z

/pulsarbot run-failure-checks

throw exception & fix dependency throw exceptions add example function add ExposeAdminClientEnabled in functions_worker.yml move module with right path style revert changes

freeznet · 2021-02-01T23:37:44Z

/pulsarbot run-failure-checks

freeznet · 2021-02-05T03:07:47Z

@codelipenghui will this pr in 2.7.1 release?

nlu90 · 2021-02-08T21:59:36Z

sorry for the late reply. @sijie

I just notice this change and am wondering the necessity of exposing the whole admin client from functions to users. Maybe we can have some discussion when you are available

lhotari · 2021-02-15T10:37:22Z

pulsar-client-admin-api/pom.xml

+    <dependencies>
+        <dependency>
+            <groupId>${project.groupId}</groupId>
+            <artifactId>pulsar-common</artifactId>
+            <version>${project.parent.version}</version>
+        </dependency>
+
+        <dependency>
+            <groupId>${project.groupId}</groupId>
+            <artifactId>pulsar-client-original</artifactId>
+            <version>${project.parent.version}</version>
+        </dependency>
+
+        <dependency>
+            <groupId>${project.groupId}</groupId>
+            <artifactId>pulsar-package-core</artifactId>
+            <version>${project.version}</version>
+        </dependency>


Is it intentional that these dependencies are included? This change seems to be causing a major regression to Pulsar IO connectors. Please see #9572 . /cc @sijie @eolivelli

The size of Pulsar IO connectors files get back to normal after changing the scope of pulsar-client-admin-api in pulsar-functions/api-java/pom.xml from <scope>compile</scope> to <scope>provided</scope>.

However I'm a bit concerned about the usefulness of the new pulsar-client-admin-api library since it's not really a standalone api dependency. It pulls in all dependencies. WDYT?

Size of https://hub.docker.com/layers/streamnative/pulsar-all/2.8.0-rc-202102022225/images/sha256-50d6a03312262d5b60efbb29e8c4a3f2245d1317ee25164e18bbd43a0d86113c?context=exploreis 2.91GB ,
on the previous day, it was 2.25GB:
https://hub.docker.com/layers/streamnative/pulsar-all/2.8.0-rc-202102012227/images/sha256-04d33735218a612dd53c7963a35bbe9d8c9a0db1625555aa501cfa95821e029f?context=explore

The ≈700MB increase most likely happened after this PR was merged.

…pache#9246) Fixes apache#8089 - add `getPulsarAdmin()` and `getPulsarAdmin(String clusterName)` in function `Context` - add implementation of above interface in `ContextImpl` - add passing `pulsarWebServiceUrl` to function runtimes - add passing `pulsarWebServiceUrl` to `LocalRunner` / `JavaInstanceStarter` - add `"--web-service-url"` parameter to `LocalRunner` admin command - add unit tests - add example function - add `exposeAdminClientEnabled` in `functions_worker.yml`

eolivelli reviewed Jan 20, 2021

View reviewed changes

freeznet mentioned this pull request Jan 20, 2021

Support pass pulsarWebServiceUrl to Java Function Runtime streamnative/function-mesh#68

Open

sijie assigned freeznet Jan 20, 2021

sijie added the area/function label Jan 20, 2021

sijie added this to the 2.8.0 milestone Jan 20, 2021

sijie reviewed Jan 20, 2021

View reviewed changes

freeznet force-pushed the freeznet/add-admin-client-to-function-context branch 2 times, most recently from e7ea3af to 3912b3c Compare January 25, 2021 13:33

freeznet force-pushed the freeznet/add-admin-client-to-function-context branch 2 times, most recently from ad59e70 to 6b9e067 Compare January 27, 2021 06:07

sijie approved these changes Jan 27, 2021

View reviewed changes

eolivelli approved these changes Jan 27, 2021

View reviewed changes

freeznet force-pushed the freeznet/add-admin-client-to-function-context branch 2 times, most recently from 1ee8f5e to 55a1dcc Compare January 30, 2021 14:49

freeznet force-pushed the freeznet/add-admin-client-to-function-context branch from c1f9684 to 5116c0d Compare February 1, 2021 03:14

freeznet added 7 commits February 1, 2021 21:05

add admin to function context

f7ae0e2

add webServiceUrl to runtime

e941d0a

add unit tests

c65e283

move admin apis to separate module

40823bc

throw exception & fix dependency throw exceptions add example function add ExposeAdminClientEnabled in functions_worker.yml move module with right path style revert changes

cleanup

65ae2d7

revert

5488f73

make optional

5016270

freeznet added 5 commits February 1, 2021 21:05

fix NPE

59d590e

remove admin-api from pulsar-functions-instance

3791953

remove shade from java-test-functions & fix ci errors

cf3a157

fix shade error

cd73ab8

fix npe

30d90e7

freeznet force-pushed the freeznet/add-admin-client-to-function-context branch from 5116c0d to 30d90e7 Compare February 1, 2021 14:49

sijie added the release/2.7.1 label Feb 2, 2021

sijie merged commit 0469dfe into apache:master Feb 2, 2021

codelipenghui removed the release/2.7.1 label Feb 4, 2021

Anonymitaet added the doc-required Your PR changes impact docs and you will update later. label Feb 7, 2021

Anonymitaet mentioned this pull request Feb 7, 2021

[Doc] Add descriptions for function runtime and context #9516

Merged

lhotari mentioned this pull request Feb 15, 2021

Exclude Pulsar Functions Worker dependencies from Pulsar IO .nar files #9572

Closed

lhotari reviewed Feb 15, 2021

View reviewed changes

This was referenced Feb 19, 2021

Reduce Pulsar IO Connectors size #9638

Closed

pulsar-client-admin-api has excessive transient dependencies #9640

Closed

sijie mentioned this pull request Feb 19, 2021

ISSUE-9640: pulsar-client-admin-api has excessive transient dependencies streamnative/pulsar-archived#2182

Closed

jerrypeng mentioned this pull request Feb 23, 2021

Include pulsar-client-admin-api in the shaded version of pulsar-client-admin #9689

Merged

Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Mar 9, 2021

freeznet mentioned this pull request Mar 9, 2021

[WIP] [fix #9640] fix pulsar-client-admin-api dependency #9842

Closed

5 tasks

lhotari mentioned this pull request May 7, 2021

[Functions] Context.getPulsarAdmin() may allow access to data from other tenants #10505

Closed

freeznet mentioned this pull request May 25, 2021

[fix #9640] remove pulsar-client-admin-api dependency : pulsar-common #10697

Closed

1 task

lhotari mentioned this pull request Jun 14, 2021

[Build] Fix java-test-functions dependencies and packaging #10918

Merged

michaeljmarshall mentioned this pull request Jul 6, 2022

Only generate Javadocs for released versions of Pulsar apache/pulsar-site#133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix #8089] expose PulsarAdmin client through Function Context #9246

[fix #8089] expose PulsarAdmin client through Function Context #9246

freeznet commented Jan 20, 2021 •

edited

Loading

eolivelli Jan 20, 2021

sijie Jan 20, 2021

freeznet Jan 21, 2021

sijie commented Jan 20, 2021

sijie left a comment

sijie Jan 20, 2021

freeznet Jan 25, 2021

freeznet commented Jan 25, 2021 •

edited

Loading

sijie left a comment

eolivelli left a comment

freeznet commented Jan 28, 2021

freeznet commented Jan 31, 2021

freeznet commented Jan 31, 2021

freeznet commented Jan 31, 2021

freeznet commented Feb 1, 2021

freeznet commented Feb 1, 2021

freeznet commented Feb 1, 2021

freeznet commented Feb 5, 2021

nlu90 commented Feb 8, 2021

lhotari Feb 15, 2021

lhotari Feb 15, 2021

lhotari Feb 15, 2021

[fix #8089] expose PulsarAdmin client through Function Context #9246

[fix #8089] expose PulsarAdmin client through Function Context #9246

Conversation

freeznet commented Jan 20, 2021 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sijie commented Jan 20, 2021

sijie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freeznet commented Jan 25, 2021 • edited Loading

sijie left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

freeznet commented Jan 28, 2021

freeznet commented Jan 31, 2021

freeznet commented Jan 31, 2021

freeznet commented Jan 31, 2021

freeznet commented Feb 1, 2021

freeznet commented Feb 1, 2021

freeznet commented Feb 1, 2021

freeznet commented Feb 5, 2021

nlu90 commented Feb 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freeznet commented Jan 20, 2021 •

edited

Loading

freeznet commented Jan 25, 2021 •

edited

Loading