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

[issue #807] dlq topic producer options #809

Merged
merged 4 commits into from
Sep 3, 2022

Conversation

zzzming
Copy link
Contributor

@zzzming zzzming commented Jul 17, 2022

Fixes #807

Motivation

To customize producer options for DLQ topics.

Modifications

Add DLQProducerOptions to consumer's DLQ policy.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

The existing DLQ test is enhanced to cover customized producer options.

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

A new producer options field, DLQProuducerOptions, is introduced for DLQ policy to govern the producer options.

ConsumerOptions{
		Topics:              topics,
		DLQ: &DLQPolicy{
			MaxDeliveries:   3,
			DeadLetterTopic: dlqTopic,
			DLQProducerOptions: ProducerOptions{
				BatchingMaxPublishDelay: 100 * time.Millisecond,
			},
		},
  • Dependencies (does it add or upgrade a dependency): no
  • The public API: yes
  • The schema: no
  • The default values of configurations: yes (the existing lz4 compression type is the default.)
  • The wire protocol: no

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (GoDocs)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

Copy link
Contributor

@pgier pgier left a comment

Choose a reason for hiding this comment

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

LGTM, just had one in-line question.


// the origin code sets to LZ4 compression with no options
// so the new design allows compression type to be overwritten but still set lz4 by default
if r.policy.DLQProducerOptions.CompressionType == NoCompression {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any cases where a user would want to explicitly set to NoCompression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation complies with the default LZ4 compression type in the previous implementation. So user cannot choose NoCompression. If NoCompression is required (I would prefer data has to be sent compressed as the original design), we might have to introduce another option to maintain backward compatibility.

@ne1llee
Copy link

ne1llee commented Jul 19, 2022

DLQ and RLQ are created together, and the same data goes first to RLQ and then to DLQ,so the producer configuration of RLQ also needs to be modified?

producer, err := r.client.CreateProducer(ProducerOptions{
Topic: r.policy.RetryLetterTopic,
CompressionType: LZ4,
BatchingMaxPublishDelay: 100 * time.Millisecond,
})

@zzzming
Copy link
Contributor Author

zzzming commented Jul 19, 2022

Added code to use the same ProducerOptions for both DLQ and RLQ.

DLQ and RLQ are created together, and the same data goes first to RLQ and then to DLQ,so the producer configuration of RLQ also needs to be modified?

producer, err := r.client.CreateProducer(ProducerOptions{
Topic: r.policy.RetryLetterTopic,
CompressionType: LZ4,
BatchingMaxPublishDelay: 100 * time.Millisecond,
})

@ne1llee
Copy link

ne1llee commented Jul 27, 2022

May I ask which version this pull will be megred in?

@zzzming zzzming requested a review from pgier August 23, 2022 16:46
@zzzming
Copy link
Contributor Author

zzzming commented Aug 23, 2022

@wolfstudy @michaeljmarshall Can you help review this PR? @ne1llee is asking when to merge this?

Copy link
Contributor

@pgier pgier left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

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.

How to change the BatchingMaxSize of DLQ
4 participants