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

[improve][client] Add optional fields to dead letter policy for configuring batching/chunking #20936

Closed
wants to merge 4 commits into from

Conversation

rindavis
Copy link

@rindavis rindavis commented Aug 6, 2023

Fixes #20934
PIP 293

Motivation

We encountered an issue when chunked messages had to be retried, like

org.apache.pulsar.client.api.PulsarClientException$InvalidMessageException: The producer mdl-pulsar-0-12 of the topic part-v1 sends a message with 7094331 bytes that exceeds 5242880 bytes

because retry letter and dead letter producers never enable chunking. Our chunked messages may be very large, so simply increasing the maximum is probably not a good option.

This change allows the client to set if they want chunking and batching for these produces. The properties are separated in retry letter and dead letter, since currently the retry letter disables batching while the dead letter enables it implicitly. (So, we did not want a single pair of new properties to always set them to the same value.)

Modifications

Edited the DeadLetterPolicy to add these new fields, and the ConsumerImpl to pick them up in the consumer and when creating the producers.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added unit test for setting the new fields, including one checking the defaults

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

We may want to update the Java client documentation to reflect these new options.

Matching PR in forked repository

PR in forked repository: rindavis#1

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Aug 6, 2023
Copy link
Author

Choose a reason for hiding this comment

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

We added 4 new properties instead of just 2 because the current settings in ConsumerImpl (next file in change set) have batching set to false for retry letter and to true (by default) for dead letter. Thus we did not want a single setting to have to be shared between both producers.

Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

We need a PIP before merging this PR.

Comment on lines 367 to 368
.retryLetterBatchingEnabled(true)
.retryLetterChunkingEnabled(true)
Copy link
Member

Choose a reason for hiding this comment

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

We couldn't enable the batching and chunking at the same time. This will throw an exception when initializing the retryLetterProducer:

checkArgument(!(conf.isBatchingEnabled() && conf.isChunkingEnabled()),
"Batching and chunking of messages can't be enabled together");

Copy link
Author

Choose a reason for hiding this comment

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

Corrected the test case. Do we need to add validation somewhere else in this changeset, or is having it in the ProducerBuilderImpl enough?

Copy link
Member

Choose a reason for hiding this comment

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

IMO, we shouldn't expose these configurations to DeadLetterPolicy. This will introduce some producer concepts to the consumer.

@jackson-chris
Copy link

Can you educate me on why a PIP is required for this change? This is a defect of existing implementation and thus not a new feature and as far as I can tell not a big change.

@RobertIndie
Copy link
Member

Can you educate me on why a PIP is required for this change? This is a defect of existing implementation and thus not a new feature and as far as I can tell not a big change.

This PR adds new configurations to the DeadLetterPolicy which is a part of public API. Therefore it introduces a new feature. Any changes to the public API should require a PIP.

@rindavis rindavis closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required Your PR changes impact docs and you will update later.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Dead Letter Policy in Consumer configuration doesn't account for chunking or batching configuration.
3 participants