Skip to content

[improve][client] PIP-409: support producer configuration for retry/dead letter topic producer #24020

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
thetumbled merged 23 commits into from
Mar 6, 2025
Merged

[improve][client] PIP-409: support producer configuration for retry/dead letter topic producer #24020

thetumbled merged 23 commits into from
Mar 6, 2025

Conversation

@thetumbled
Copy link
Member

@thetumbled thetumbled commented Feb 25, 2025 •
edited by lhotari
Loading

PIP: #24022
Fixes #19463
Fixes #21954

Motivation

Currently, we don't support configure the producer of retry/dead letter topic. But enable the chunk message feature
and disable the batch configuration in hard code, which can't handle many situations. For example, when the throughput
of message of retry topic become considerable, the resource consumed by the un-batched messages is pretty large.
There is no reason that we disable the batch message feature.

For better control for the retry/dead letter topic feature, we can support configuration for the producer of
retry/dead letter topic.

Modifications

Support configuration for the producer of retry/dead letter topic.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: thetumbled#71

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Feb 25, 2025
@thetumbled thetumbled mentioned this pull request Feb 25, 2025
Merged
4 tasks
@thetumbled thetumbled mentioned this pull request Feb 25, 2025
Closed
@thetumbled
Copy link
Member Author

thetumbled commented Feb 28, 2025

The pip document has been merged, we can continue to review this pr. Thanks for your review. @BewareMyPower @nodece @dao-jun @codelipenghui @shibd @Technoboy-

lhotari
lhotari requested changes Feb 28, 2025
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Instead of using java.util.function.Function, I'd suggest using a richer interface so that more context parameters can be added besides the topic name. This pattern would also allow adding more context parameters later without breaking the API in backwards incompatible ways.

@lhotari
Copy link
Member

lhotari commented Feb 28, 2025

Great work, @thetumbled. Thanks for making this happen. The PIP-409 solution will help resolve long time issues such as #19463 and #21954.

This was referenced Feb 28, 2025
Closed
Closed
Closed
lhotari
lhotari approved these changes Mar 5, 2025
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM, good work @thetumbled . Great to see this finally solving many use cases.

@thetumbled
Copy link
Member Author

thetumbled commented Mar 5, 2025

LGTM, good work @thetumbled . Great to see this finally solving many use cases.

Thanks for your review and contributions!

lhotari
lhotari requested changes Mar 5, 2025
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

One remaining issue about naming. Commented added.

@thetumbled
Copy link
Member Author

thetumbled commented Mar 5, 2025

One remaining issue about naming. Commented added.

fixed, thanks.

@thetumbled thetumbled requested a review from lhotari March 5, 2025 10:06
lhotari
lhotari reviewed Mar 5, 2025
View reviewed changes
@thetumbled thetumbled requested a review from lhotari March 6, 2025 02:27
@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 55.55556% with 8 lines in your changes missing coverage. Please review.

Project coverage is 74.20%. Comparing base (bbc6224) to head (089cb74).
Report is 951 commits behind head on master.

Files with missing lines Patch % Lines
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 71.42% 3 Missing and 1 partial ⚠️
...ent/impl/DeadLetterProducerBuilderContextImpl.java 0.00% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24020      +/-   ##
============================================
+ Coverage     73.57%   74.20%   +0.62%     
+ Complexity    32624    32031     -593     
============================================
  Files          1877     1862      -15     
  Lines        139502   144225    +4723     
  Branches      15299    16433    +1134     
============================================
+ Hits         102638   107015    +4377     
+ Misses        28908    28764     -144     
- Partials       7956     8446     +490
Flag Coverage Δ
inttests 26.67% <38.88%> (+2.09%) ⬆️
systests 23.08% <0.00%> (-1.24%) ⬇️
unittests 73.71% <55.55%> (+0.87%) ⬆️

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

Files with missing lines Coverage Δ
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 78.85% <71.42%> (+1.28%) ⬆️
...ent/impl/DeadLetterProducerBuilderContextImpl.java 0.00% <0.00%> (ø)

... and 1054 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thetumbled thetumbled added release/4.0.4 area/client type/feature The PR added a new feature or issue requested a new feature labels Mar 6, 2025
@thetumbled thetumbled merged commit 302c1d5 into apache:master Mar 6, 2025
54 of 55 checks passed
@thetumbled thetumbled mentioned this pull request Mar 12, 2025
Merged
15 tasks
lhotari added a commit that referenced this pull request Mar 17, 2025
@thetumbled @lhotari
[improve][client] PIP-409: support producer configuration for retry/d…
6d47526 
…ead letter topic producer (#24020)

Co-authored-by: Lari Hotari <lhotari@apache.org>
(cherry picked from commit 302c1d5)
@thetumbled thetumbled mentioned this pull request Mar 18, 2025
Merged
2 tasks
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 29, 2025
@thetumbled @lhotari @nikhil-ctds
[improve][client] PIP-409: support producer configuration for retry/d…
cfba270 
…ead letter topic producer (apache#24020)

Co-authored-by: Lari Hotari <lhotari@apache.org>
(cherry picked from commit 302c1d5)
(cherry picked from commit 6d47526)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 31, 2025
@thetumbled @lhotari @srinath-ctds
[improve][client] PIP-409: support producer configuration for retry/d…
5546d45 
…ead letter topic producer (apache#24020)

Co-authored-by: Lari Hotari <lhotari@apache.org>
(cherry picked from commit 302c1d5)
(cherry picked from commit 6d47526)
nborisov pushed a commit to nborisov/pulsar that referenced this pull request Sep 12, 2025
@thetumbled @lhotari
[improve][client] PIP-409: support producer configuration for retry/d…
6186fe0 
…ead letter topic producer (apache#24020)

Co-authored-by: Lari Hotari <lhotari@apache.org>
(cherry picked from commit 302c1d5)
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025
@thetumbled @lhotari @walkinggo
[improve][client] PIP-409: support producer configuration for retry/d…
719d08b 
…ead letter topic producer (apache#24020)

Co-authored-by: Lari Hotari <lhotari@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nodece nodece nodece left review comments

@lhotari lhotari lhotari approved these changes

@eolivelli eolivelli Awaiting requested review from eolivelli

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@crossoverJie crossoverJie Awaiting requested review from crossoverJie

@BewareMyPower BewareMyPower Awaiting requested review from BewareMyPower

@dao-jun dao-jun Awaiting requested review from dao-jun

@Demogorgon314 Demogorgon314 Awaiting requested review from Demogorgon314

@poorbarcode poorbarcode Awaiting requested review from poorbarcode

Assignees

No one assigned

Labels

area/client cherry-picked/branch-4.0 doc-required Your PR changes impact docs and you will update later. PIP release/4.0.4 type/feature The PR added a new feature or issue requested a new feature

Projects

None yet

Milestone

No milestone

5 participants

@thetumbled @lhotari @codecov-commenter @nodece @BewareMyPower