[improve][fn] Support configure compression type by jiangpengcheng · Pull Request #19470 · apache/pulsar

jiangpengcheng · 2023-02-09T02:24:14Z

Fixes #19462

Motivation

Modifications

Verifying this change

Make sure that the change passes the CI checks.

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

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository: jiangpengcheng#6

jiangpengcheng · 2023-02-10T07:34:13Z

/pulsarbot rerun-failure-checks

jiangpengcheng · 2023-02-10T10:18:55Z

/pulsarbot rerun-failure-checks

codecov-commenter · 2023-02-10T19:02:40Z

Codecov Report

Merging #19470 (10b22b4) into master (e286339) will decrease coverage by 28.96%.
The diff coverage is 9.49%.

@@              Coverage Diff              @@
##             master   #19470       +/-   ##
=============================================
- Coverage     60.44%   31.49%   -28.96%     
+ Complexity     3494      293     -3201     
=============================================
  Files          1832     1672      -160     
  Lines        135153   132568     -2585     
  Branches      14871    15211      +340     
=============================================
- Hits          81693    41751    -39942     
- Misses        45869    84681    +38812     
+ Partials       7591     6136     -1455

Flag	Coverage Δ
inttests	`25.81% <5.78%> (+1.14%)`	⬆️
systests	`25.13% <10.47%> (?)`
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...lance/extensions/ExtensibleLoadManagerWrapper.java	`0.00% <0.00%> (ø)`
...ns/channel/ServiceUnitStateCompactionStrategy.java	`8.16% <0.00%> (-5.18%)`	⬇️
...lance/extensions/channel/ServiceUnitStateData.java	`0.00% <0.00%> (ø)`
.../pulsar/compaction/StrategicTwoPhaseCompactor.java	`0.00% <0.00%> (-76.31%)`	⬇️
...java/org/apache/pulsar/admin/cli/CmdFunctions.java	`0.00% <0.00%> (-47.21%)`	⬇️
...ar/functions/source/MultiConsumerPulsarSource.java	`86.84% <0.00%> (-7.61%)`	⬇️
...r/functions/source/SingleConsumerPulsarSource.java	`76.66% <0.00%> (-23.34%)`	⬇️
...xtensions/channel/ServiceUnitStateChannelImpl.java	`0.58% <0.34%> (-0.08%)`	⬇️
.../apache/pulsar/functions/utils/FunctionCommon.java	`33.46% <12.50%> (-22.42%)`	⬇️
...a/org/apache/pulsar/functions/sink/PulsarSink.java	`47.24% <33.33%> (-7.39%)`	⬇️
... and 1355 more

jiangpengcheng · 2023-02-22T06:24:43Z

/pulsarbot rerun-failure-checks

nlu90 · 2023-02-24T05:12:45Z

@jiangpengcheng
Since this PR involves protobuf change, let's send an email in the mailing list to let folks in the community know about it.

jiangpengcheng · 2023-02-24T06:17:15Z

/pulsarbot rerun-failure-checks

jiangpengcheng · 2023-03-03T01:05:41Z

/pulsarbot rerun-failure-checks

jiangpengcheng · 2023-03-07T03:13:18Z

/pulsarbot rerun-failure-checks

jiangpengcheng · 2023-03-07T07:11:52Z

/pulsarbot rerun-failure-checks

tisonkun

Thanks for your contribution @jiangpengcheng! Generally looks good.

Comments below:

I don't find changes to Go Functions, do you update it later or it's irrelevant?
@momo-jun maybe we should update the document for this new feature. Although it can be a light one, the author marks it as doc-required. But I'm unsure what page is suitable for such changes.

momo-jun · 2023-03-08T06:16:12Z

@tisonkun thanks for considering the doc development.
@jiangpengcheng To scope the incremental doc updates, I think at least we have a new config that needs to be added to this topic. Can you please confirm and advise? It would be great if you could analyze the user impact of this improvement and involve the doc updates within this PR.

jiangpengcheng · 2023-03-09T01:19:27Z

@momo-jun a new field CompressionType should be added to ProducerConfig: https://pulsar.apache.org/docs/next/functions-cli/#producerconfig,

the type is String and available values are: None, LZ4, ZLIB, ZSTD and SNAPPY, and default to LZ4

jiangpengcheng · 2023-03-09T01:20:46Z

Thanks for your contribution @jiangpengcheng! Generally looks good.

Comments below:

I don't find changes to Go Functions, do you update it later or it's irrelevant?

@momo-jun maybe we should update the document for this new feature. Although it can be a light one, the author marks it as doc-required. But I'm unsure what page is suitable for such changes.

added go support and suggestions to docs update

but the ci(Go function style check) complains about using the deprecated AutoAck, do you know how to ignore it?

momo-jun · 2023-03-09T07:34:49Z

the type is String and available values are: None, LZ4, ZLIB, ZSTD and SNAPPY, and default to LZ4

@jiangpengcheng thanks for the input. I've drafted apache/pulsar-site#461 to add the docs.
Only one concern - the default config of compression type on client side, e.g., Java producers, is None (no compression), which is not aligned with functions. Is there any risk here?

nlu90 · 2023-03-09T17:31:21Z

/pulsarbot rerun-failure-checks

jiangpengcheng · 2023-03-10T08:37:52Z

the type is String and available values are: None, LZ4, ZLIB, ZSTD and SNAPPY, and default to LZ4

@jiangpengcheng thanks for the input. I've drafted apache/pulsar-site#461 to add the docs. Only one concern - the default config of compression type on client side, e.g., Java producers, is None (no compression), which is not aligned with functions. Is there any risk here?

There is no risk, just for back compatibility since current pulsar functions are using LZ4 as compression type.

One thing I did wrong is that the None should be NONE instead

jiangpengcheng · 2023-03-10T08:38:04Z

/pulsarbot rerun-failure-checks

jiangpengcheng · 2023-03-13T00:56:55Z

/pulsarbot rerun-failure-checks

michaeljmarshall · 2023-03-17T16:53:40Z

This updates the function proto. Shouldn't we have had a PIP?

nlu90 · 2023-03-20T16:53:29Z

Hi @michaeljmarshall, a mail thread sent on February 28 discussed the change.
PTAL: https://lists.apache.org/thread/8z849bhp79ob23kf1p21f6kq7rd6bljq

michaeljmarshall · 2023-03-21T04:49:19Z

Thanks for that link @nlu90! Looks good to me.

- change was made in apache#19470 - master branch build fails with "SA1019: instanceConf.funcDetails.AutoAck is deprecated: Do not use."

lhotari · 2023-03-23T07:36:48Z

This PR broke the master branch. Please review and merge #19904 asap to fix master.
The reason why this PR was mergeable is due to a CI bug #19905 .

- `go generate ./...` from repo root or - go run internal/update_proto.go <release tag> - fixes #7 Pending sync of changes to apache/pulsar#19470

lhotari · 2024-06-20T07:46:06Z

the type is String and available values are: None, LZ4, ZLIB, ZSTD and SNAPPY, and default to LZ4

@jiangpengcheng thanks for the input. I've drafted apache/pulsar-site#461 to add the docs. Only one concern - the default config of compression type on client side, e.g., Java producers, is None (no compression), which is not aligned with functions. Is there any risk here?

There is no risk, just for back compatibility since current pulsar functions are using LZ4 as compression type.

One thing I did wrong is that the None should be NONE instead

@jiangpengcheng @nlu90 This PR introduced a change to the compression type default. Only when the producer config is provided, it will use the LZ4 compression default. The producer config is optional and that's why compression will now be NONE by default when the producer config isn't provided.
Producers are created in 2 locations in the Pulsar Functions code. The other location still has LZ4 compression by default.
There's another issue #22948 which I'm currently working on. I'm not changing the behavior of the compression default in that change, but unifying the configuration so that compression type can be configured also for the producers created in ContextImpl.

github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Feb 9, 2023

jiangpengcheng changed the title ~~Support compression~~ [improve][fn] Support compression Feb 9, 2023

jiangpengcheng changed the title ~~[improve][fn] Support compression~~ [improve][fn] Support configure compression Feb 9, 2023

jiangpengcheng changed the title ~~[improve][fn] Support configure compression~~ [improve][fn] Support configure compression type Feb 9, 2023

freeznet added the ready-to-test label Feb 10, 2023

nlu90 assigned jiangpengcheng Feb 22, 2023

nlu90 requested a review from freeznet February 22, 2023 00:01

jiangpengcheng force-pushed the support_compression branch from d8976c3 to c53c47a Compare February 22, 2023 01:29

nlu90 approved these changes Feb 24, 2023

View reviewed changes

jiangpengcheng force-pushed the support_compression branch from c53c47a to a954922 Compare February 24, 2023 03:50

codelipenghui closed this Mar 8, 2023

codelipenghui reopened this Mar 8, 2023

tisonkun reviewed Mar 8, 2023

View reviewed changes

jiangpengcheng added 5 commits March 9, 2023 08:10

[improve][fn] Add compressionType parameter

161b398

Fix tests

a0e5821

Fix error

3dc9da0

Use LZ4 as default value

7891505

Support go instance

3c6b316

jiangpengcheng force-pushed the support_compression branch from 10b22b4 to 3c6b316 Compare March 9, 2023 01:13

Update license header

5c5afaf

momo-jun mentioned this pull request Mar 9, 2023

[feat][doc] Add new configs skipToLatest and compressionType apache/pulsar-site#461

Merged

4 tasks

nlu90 closed this Mar 10, 2023

nlu90 reopened this Mar 10, 2023

nlu90 merged commit e0098ee into apache:master Mar 17, 2023

Technoboy- added this to the 3.0.0 milestone Mar 19, 2023

momo-jun added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Mar 22, 2023

lhotari mentioned this pull request Mar 23, 2023

[fix][fn] Revert change to deprecation since it broke the master branch #19904

Merged

4 tasks

flowchartsman added a commit to flowchartsman/pfunc that referenced this pull request May 29, 2023

Allow update for protos from release tag

cc20c37

- `go generate ./...` from repo root or - go run internal/update_proto.go <release tag> - fixes #7 Pending sync of changes to apache/pulsar#19470

lhotari mentioned this pull request Jun 20, 2024

[fix][fn] Support compression type and crypto config for all producers in Functions and Connectors #22950

Merged

4 tasks

Conversation

jiangpengcheng commented Feb 9, 2023

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Uh oh!

jiangpengcheng commented Feb 10, 2023

Uh oh!

jiangpengcheng commented Feb 10, 2023

Uh oh!

codecov-commenter commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jiangpengcheng commented Feb 22, 2023

Uh oh!

nlu90 commented Feb 24, 2023

Uh oh!

jiangpengcheng commented Feb 24, 2023

Uh oh!

jiangpengcheng commented Mar 3, 2023

Uh oh!

jiangpengcheng commented Mar 7, 2023

Uh oh!

jiangpengcheng commented Mar 7, 2023

Uh oh!

tisonkun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

momo-jun commented Mar 8, 2023

Uh oh!

jiangpengcheng commented Mar 9, 2023

Uh oh!

jiangpengcheng commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

momo-jun commented Mar 9, 2023

Uh oh!

nlu90 commented Mar 9, 2023

Uh oh!

jiangpengcheng commented Mar 10, 2023

Uh oh!

jiangpengcheng commented Mar 10, 2023

Uh oh!

jiangpengcheng commented Mar 13, 2023

Uh oh!

michaeljmarshall commented Mar 17, 2023

Uh oh!

nlu90 commented Mar 20, 2023

Uh oh!

michaeljmarshall commented Mar 21, 2023

Uh oh!

lhotari commented Mar 23, 2023

Uh oh!

lhotari commented Jun 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

codecov-commenter commented Feb 10, 2023 •

edited

Loading

tisonkun left a comment •

edited

Loading

jiangpengcheng commented Mar 9, 2023 •

edited

Loading