Skip to content

Issue #9123 Go Functions #9124

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
Merged
sijie merged 1 commit into from
Jan 8, 2021
Merged

Issue #9123 Go Functions #9124

sijie merged 1 commit into from
Jan 8, 2021

Conversation

@flowchartsman
Copy link
Contributor

@flowchartsman flowchartsman commented Jan 5, 2021

Fixes #9123

Modifications

Check for nil ProducerSpec first

Verifying this change

as in #9123

@sijie
Copy link
Member

sijie commented Jan 6, 2021

@flowchartsman Can you rebase to the latest master to include recent changes that fix the broken master?

@sijie
Copy link
Member

sijie commented Jan 6, 2021

@freeznet Can you review this change?

@sijie sijie added this to the 2.8.0 milestone Jan 6, 2021
@freeznet
Copy link
Contributor

freeznet commented Jan 6, 2021

@flowchartsman @sijie I have some findings related to #8761.

Currently go functions get KeyBasedBatchBuilder support with #8761, but the batchBuilderType is not exposed as an individual argument in pulsar-function-go/conf/conf.go https://github.com/apache/pulsar/blob/master/pulsar-function-go/conf/conf.go, so user cannot pass batchBuilderType to function, and batchBuilder is not usable. Additional, ProducerSpec from SinkSpec are not initialized properly here: https://github.com/apache/pulsar/blob/master/pulsar-function-go/pf/instanceConf.go#L87-L90, and this will lead to #9123, user will get nil exception when access to instanceConf.funcDetails.Sink.ProducerSpec.

@flowchartsman
Copy link
Contributor Author

flowchartsman commented Jan 6, 2021 •
edited
Loading

@freeznet, it seems like what you're saying is that my fix is a band-aid at a lower level, and that the other changes would fix this the right way, is that correct? If so, I think I should close this PR and we should make a new one that correctly fixes all of the issues you mention.

@freeznet
Copy link
Contributor

freeznet commented Jan 7, 2021 •
edited
Loading

@flowchartsman I think this PR is great and LGTM since it add nil check for pointer, and the nil check is always welcome. But as you said, we should work on another PR to make BatchBuilder exposed to user properly.

@flowchartsman flowchartsman force-pushed the fix-go-func-nil-producerspec branch from 84bc333 to b78807e Compare January 8, 2021 00:55
@flowchartsman
Copy link
Contributor Author

flowchartsman commented Jan 8, 2021

@sijie should be rebased now, I believe

@sijie
Copy link
Member

sijie commented Jan 8, 2021

@flowchartsman Thank you!

@sijie
Copy link
Member

sijie commented Jan 8, 2021

/pulsarbot run-failure-checks

@sijie sijie merged commit 3b6fa26 into apache:master Jan 8, 2021
codelipenghui pushed a commit that referenced this pull request Jan 14, 2021
@flowchartsman @codelipenghui
check for nil producerspec first (#9124)
432b170 
Fixes #9123

Check for nil ProducerSpec first

as in #9123

(cherry picked from commit 3b6fa26)
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jan 14, 2021
freeznet pushed a commit to streamnative/pulsar-archived that referenced this pull request Jan 14, 2021
@flowchartsman @freeznet
check for nil producerspec first (apache#9124)
ccbdc95 
Fixes apache#9123

Check for nil ProducerSpec first

as in apache#9123

(cherry picked from commit 3b6fa26)
eolivelli added a commit to datastax/pulsar that referenced this pull request May 18, 2021
@eolivelli
Revert "check for nil producerspec first (apache#9124)"
36a0f89 
This reverts commit 3b6fa26.
@gaoran10 gaoran10 mentioned this pull request Sep 10, 2021
Closed
gaoran10 added a commit to gaoran10/pulsar that referenced this pull request Sep 13, 2021
@gaoran10
upgrade pulsar-go-client and revert apache#9124 from branch-2.7
2cff49f
@gaoran10 gaoran10 mentioned this pull request Sep 13, 2021
Merged
codelipenghui pushed a commit that referenced this pull request Sep 14, 2021
@gaoran10
[Branch-2.7][Build] Use ubuntu 20.04 as docker image base (#12017)
259c698 
### Motivation

Currently, the docker is from `openjdk:8-jdk-slim `, it has a new release a few days ago and the Linux version was changed to `Impish`, it didn't support install `python3.7` by apt-get tool, the minimum Python version is 3.9, so we need to change the python version or use a different Linux release, I cherry-pick some commits from branch master to build docker image from the `ubuntu:20.04`.

Mainly related PRs:

1. #11026
2. #11623
3. #11862

### Modifications

Build docker image from the `ubuntu:20.04`.

upgrade pulsar-go-client from `0.2.0` to `0.6.0` and rever #9124 from branch-2.7
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 22, 2022
@gaoran10 @nicoloboschi
[Branch-2.7][Build] Use ubuntu 20.04 as docker image base (apache#12017)
13e0bc4 
Currently, the docker is from `openjdk:8-jdk-slim `, it has a new release a few days ago and the Linux version was changed to `Impish`, it didn't support install `python3.7` by apt-get tool, the minimum Python version is 3.9, so we need to change the python version or use a different Linux release, I cherry-pick some commits from branch master to build docker image from the `ubuntu:20.04`.

Mainly related PRs:

1. apache#11026
2. apache#11623
3. apache#11862

Build docker image from the `ubuntu:20.04`.

upgrade pulsar-go-client from `0.2.0` to `0.6.0` and rever apache#9124 from branch-2.7

(cherry picked from commit 259c698)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 22, 2022
@gaoran10 @nicoloboschi
[Branch-2.7][Build] Use ubuntu 20.04 as docker image base (apache#12017)
d313a0b 
Currently, the docker is from `openjdk:8-jdk-slim `, it has a new release a few days ago and the Linux version was changed to `Impish`, it didn't support install `python3.7` by apt-get tool, the minimum Python version is 3.9, so we need to change the python version or use a different Linux release, I cherry-pick some commits from branch master to build docker image from the `ubuntu:20.04`.

Mainly related PRs:

1. apache#11026
2. apache#11623
3. apache#11862

Build docker image from the `ubuntu:20.04`.

upgrade pulsar-go-client from `0.2.0` to `0.6.0` and rever apache#9124 from branch-2.7

(cherry picked from commit 259c698)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sijie sijie sijie approved these changes

Assignees

@flowchartsman flowchartsman

Labels

area/function cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.1

Projects

None yet

Milestone

2.8.0

Development

Successfully merging this pull request may close these issues.

[Go Functions] publishfunc example is broken

4 participants

@flowchartsman @sijie @freeznet @codelipenghui