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.

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

[PIP 86][Function] add funtion api to support preload and release external resources #13205

Merged
merged 11 commits into from
Feb 14, 2022

Conversation

wangjialing218
Copy link
Contributor

Motivation

This patch is based on #11112, I have talked with @wuchenxi123 and I will continue the work for this feature.

Modifications

Introduce RichFunction interface extends Function, which provide setup and tearDown API.
setup is called only once when function instance started. tearDown is called when function instance closed.
User could use these interface to initialize and release their external resources such as RedisClient.

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

If yes was chosen, please highlight the changes

  • The public API: ( yes)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • doc-required
    I think we need to add chapter to tell user how to use the API in functions-develop.md

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Dec 9, 2021
@wangjialing218
Copy link
Contributor Author

@nlu90 @eolivelli could you please have a check

@nlu90
Copy link
Member

nlu90 commented Dec 10, 2021

@wangjialing218 Did you call a vote for the PIP-86 in the dev@pulsar mailing list? Also you may want to update the PIP-86 content to reflect the newest idea.

It's good if we can get feedback from the community. And then start the review work.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Overall is good.
Please add unit tests and integration tests.

There is a duplicate PR for this PIP, please close the old one

Copy link
Member

@nlu90 nlu90 left a comment

Choose a reason for hiding this comment

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

The PIP Vote has been passed.

And some updates in the PIP-86 needed to reflect the actual changes we added here.

@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@wangjialing218
Copy link
Contributor Author

@nlu90 @eolivelli please have a check again

Copy link
Member

@nlu90 nlu90 left a comment

Choose a reason for hiding this comment

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

LGTM

@merlimat merlimat added this to the 2.10.0 milestone Feb 2, 2022
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

The change looks good.
But in your test your are not testing that the new methods are executed.
I suggest to add some static variables in the function in order to be able to check that the function runs

@wangjialing218
Copy link
Contributor Author

wangjialing218 commented Feb 7, 2022

The change looks good. But in your test your are not testing that the new methods are executed. I suggest to add some static variables in the function in order to be able to check that the function runs

@eolivelli I tried to add static variables in function but failed to get correct value in test code, may be caused by different class loader, could you give some suggestion?
Anyway, in the test function, process() will throw exception if initialize() did not run, and test code could check whether all messages received.

@nlu90
Copy link
Member

nlu90 commented Feb 11, 2022

@eolivelli Could you help take a look again when you are available?

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

@codelipenghui codelipenghui merged commit 7ab2987 into apache:master Feb 14, 2022
@Anonymitaet Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Feb 18, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
…ernal resources (apache#13205)

### Motivation

This patch is based on apache#11112, I have talked with @wuchenxi123 and I will continue the work for this feature.

### Modifications

Introduce `RichFunction` interface extends `Function`,  which provide `setup` and `tearDown` API.
`setup` is called only once when function instance started. `tearDown` is called when function instance closed.
User could use these interface to initialize and release their external resources such as RedisClient.

### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
  - The public API: ( **yes**)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants