Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Function runtime pluggable #5463

Merged
merged 5 commits into from
Nov 2, 2019
Merged

Function runtime pluggable #5463

merged 5 commits into from
Nov 2, 2019

Conversation

jerrypeng
Copy link
Contributor

@jerrypeng jerrypeng commented Oct 25, 2019 •
edited
Loading

Master Issue: #4176

Motivation

Currently, Pulsar Function runtime does not expose a clean interface to make it pluggable.

Modifications

Make Pulsar Function Runtime Factory pluggable to make specifying Function Runtimes more flexible and allow the development of future runtimes to be more smooth since a complete interface is extracted

Also added additional tests, clean up of code, and BC tests

@jerrypeng jerrypeng added this to the 2.5.0 milestone Oct 25, 2019
@jerrypeng jerrypeng self-assigned this Oct 25, 2019
@jerrypeng
Copy link
Contributor Author

rerun integration tests

1 similar comment
@jerrypeng
Copy link
Contributor Author

rerun integration tests

@sijie sijie requested a review from wolfstudy October 26, 2019 06:11
@sijie
Copy link
Member

sijie commented Oct 26, 2019

@wolfstudy please help review this function runtime interface

@jerrypeng
Copy link
Contributor Author

@sijie @wolfstudy @merlimat please review

merlimat
merlimat approved these changes Oct 31, 2019
View reviewed changes
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

sijie
sijie requested changes Oct 31, 2019
View reviewed changes
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

@jerrypeng overall looks good to me. I have one concern about the configuration settings. I would like to see a smooth upgrade for the users.

pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java
@@ -328,162 +332,20 @@ public boolean getTlsEnabled() {
}


@Data
Copy link
Member

Choose a reason for hiding this comment

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

Removing configuration is usually treated as a breaking change. Can we keep the old configuration settings for one release but deprecate them? Then after a few more releases, we can remove the settings. Otherwise, it is painful for people to upgrade a cluster.

Copy link
Contributor Author

@jerrypeng jerrypeng Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@sijie I didn't remove them, I moved them down to here:

https://github.com/apache/pulsar/pull/5463/files#diff-a26c05f28ea3e67193e9ad83c63f687fR435

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is also a test to make sure BC is preserved:

https://github.com/apache/pulsar/pull/5463/files#diff-7eb172c16a86f29eeec8f23f17993cdcR829

@jerrypeng jerrypeng requested a review from sijie October 31, 2019 18:58
@jerrypeng
Copy link
Contributor Author

rerun java8 tests

@jerrypeng
Copy link
Contributor Author

rerun integration tests

@aahmed-se
Copy link
Contributor

@jerrypeng

org/apache/pulsar/client/impl/ReaderBuilderImpl
Stacktrace
java.lang.NoClassDefFoundError: org/apache/pulsar/client/impl/ReaderBuilderImpl
	at org.apache.pulsar.client.impl.PulsarClientImpl.newReader(PulsarClientImpl.java:192)
	at org.apache.pulsar.tests.integration.functions.PulsarFunctionsTest.testWindowFunction(PulsarFunctionsTest.java:1019)
	at org.apache.pulsar.tests.integration.functions.PulsarFunctionsTest.testSlidingCountWindowTest(PulsarFunctionsTest.java:1084)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:124)
	at org.testng.internal.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:54)
	at org.testng.internal.InvokeMethodRunnable.run(InvokeMethodRunnable.java:44)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

@jerrypeng
Copy link
Contributor Author

rerun integration tests

@jerrypeng
Copy link
Contributor Author

@sijie please take a look again. I have replied to your comments.

sijie
sijie approved these changes Nov 2, 2019
View reviewed changes
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

@jerrypeng thank you for your clarification.

One more thought is moving the plugin to a NAR package in the future. This would make Pulsar more extensible without worrying about dependencies conflicts if people would like to develop a new runtime for Pulsar.

@aahmed-se aahmed-se merged commit 9728d54 into apache:master Nov 2, 2019
@jerrypeng
Copy link
Contributor Author

@sijie that is something we should definitely think about. Though I would suggest we have discussion on how to handle plugins in general in Pulsar. Perhaps we should implement or standardize on some sort of plugin loading framework.

@sijie
Copy link
Member

sijie commented Nov 2, 2019

@jerrypeng agree. we have used NAR for functions, connectors, offloaders and protocol handlers. I think we can continue that route.

jerrypeng added a commit to jerrypeng/incubator-pulsar that referenced this pull request Dec 17, 2019
@jerrypeng
Function runtime pluggable (apache#5463)
29aaaa1 
* Allow Pulsar Function Runtimes to be pluggable

* cleaning up
This was referenced Dec 27, 2019
Closed
Closed
@RobertIndie RobertIndie mentioned this pull request Mar 16, 2021
Merged
sijie pushed a commit that referenced this pull request Mar 17, 2021
@RobertIndie
[Doc] Use pluggable function runtime setting in the doc (#9911)
5cc89fc 
### Motivation
In #5463, we have added the pluggable function runtime setting since 2.5.0. It is better if we can use this feature in the doc.

### Modifications

Update the doc to use pluggable function runtime.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlimat merlimat merlimat approved these changes

@sijie sijie sijie approved these changes

@aahmed-se aahmed-se aahmed-se approved these changes

@srkukarni srkukarni Awaiting requested review from srkukarni

@wolfstudy wolfstudy Awaiting requested review from wolfstudy

Assignees

@jerrypeng jerrypeng

Labels
area/function
Projects
None yet
Milestone
2.5.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jerrypeng @sijie @aahmed-se @merlimat