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

fix the default retry letter and dead letter topic name #10129

Merged
merged 4 commits into from
May 15, 2021

Conversation

wangjialing218
Copy link
Contributor

Motivation

Fixes #9327

Modifications

Correct the default retry letter and dead letter topic name depend on full topic name

@merlimat
Copy link
Contributor

merlimat commented Apr 3, 2021

The problem with this change is that by renaming the default, a user who upgrades the client will suddenly have the DLQ messages routed to a different topic, and this could potentially lead to loose these messages.

@wangjialing218
Copy link
Contributor Author

Currently, the default name of dead letter topic is {TopicName}-{Subscription}-DLQ while enableRetry(true) is not speicified, but when enableRetry(true) is speicified, the default name of dead letter topic is {NamespaceName}-{Subscription}-DLQ, which will be confused.

@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

@wangjialing218 We need to handle the compatibility issues when users upgrade to the new Pulsar Client. As @merlimat mentioned, this will lead to data loss when upgrading to the new Pulsar Client.

@wangjialing218
Copy link
Contributor Author

wangjialing218 commented Apr 7, 2021

@merlimat @codelipenghui Currently a cosumer with retry message enable could receive another topic's retry message due to the same retry letter topic name.
Consider a consumer subscribe to a topic with retry topic enable, and another consumer subscribe to another topic under same namespace with same subscription name, both of the two consumers will subscribe to the retry topic {NamespaceName}-{Subscription}-RETRY, and they could receive the retry messages from each other.
Changing default name of retry letter topic will not cause compatibility issues when users upgrade pulsar client, because consumer will automatic subscribe the retry letter topic at builder time.
For dead letter topic, since user need to specify the topic name, we need to consider how to handle the compatibility issue. such as only change the default name of retry letter name?

@codelipenghui
Copy link
Contributor

@wangjialing218 This will cause compatible issues because users might topic1 and topic2 as the retry topic name and DLQ name, but after upgrade to the new client version, the client will subscribe to a new retry topic topic3 and the message which need to send to the DLQ will send to a new topic topic4, this is why we need to consider the compatible issue.

@wangjialing218
Copy link
Contributor Author

@codelipenghui @merlimat Add compatible check. If old name of DLQ and retry topic exists, using old topics.

@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@wangjialing218
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie sijie added this to the 2.8.0 milestone Apr 13, 2021
@sijie
Copy link
Member

sijie commented May 4, 2021

@eolivelli please review this PR again

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I would like to point out that with this change it is possibile to upgrade to the new version safely but in case of downgrade the user will probably have surprise.

Nice work. Thanks for your contribution

@eolivelli
Copy link
Contributor

@codelipenghui PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retry and Dead letter topics are generated with wrong names when retry is enabled
5 participants