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 #4897] Implement retry consuming based on pulsar message middleware #4898

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

HarshSawarkar
Copy link
Contributor

@HarshSawarkar HarshSawarkar commented May 18, 2024

Fixes #4897

Motivation

This implementation will add retry consuming on Pulsar message middleware

Modifications

Implemented the code for supporting retry based on Pulsar

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • 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

.enableRetry(true)
.deadLetterPolicy(DeadLetterPolicy.builder()
.deadLetterTopic(dlqTopic)
.retryLetterTopic(retryTopic)
Copy link
Contributor

Choose a reason for hiding this comment

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

You have defined the retry topic here, why do you still need to manually send the message to the retry topic in PulsarRetryStrategyImpl? Doesn't Pulsar automatically send failed messages to the retry topic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

} catch (Exception e) {
ackConsumer.negativeAcknowledge(msg);
try {
ackConsumer.reconsumeLater(msg, 5, TimeUnit.SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

For negativeAcknowledge() and reconsumeLater(), the former will automatically re-consume messages from the retry topic retryTopic, while the latter will re-consume messages from the current topic subTopic. Why do you need to use both? Have I misunderstood?

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 negativeAcknowledge() method enables the redelivery of failed messages by sending them back to the main queue with the main topic. The reconsumeLater() method, on the other hand, handles retries by moving the failed messages to a special retry topic stored in a retry queue, where they will be reprocessed after a specified delay.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the correction. So why? There are two retries here, and the default retry of EventMesh. So why does Pulsar's retry need to be set to three times instead of just once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compared with negative acknowledgment, retry letter topic is more suitable for messages that require a large number of retries with a configurable retry interval.The messages in the retry letter topic are persisted to BookKeeper, while messages that need to be retried due to negative acknowledgment are cached on the client side.Negative acks and retry/DLQ do work together

Copy link
Contributor

Choose a reason for hiding this comment

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

Negative acks and retry/DLQ do work together

Are you sure it's a collaboration and not duplicate consumption? If you are sure, could you explain this: how can it ensure that only one of the mechanisms will be triggered for retries when there are failed messages, given that both mechanisms exist simultaneously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I had a misunderstanding. The negative acknowledgment will cause duplicate consumption with active retry. I have fixed it.

…tegy-based-on-pulsar-message-middleware' into apache#4556-Implement-retry-strategy-based-on-pulsar-message-middleware
Copy link
Contributor

@pandaapo pandaapo left a comment

Choose a reason for hiding this comment

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

I found that the original design of the retry module seems to have some problems, but I'm not sure yet. I need to close issue #4556 first. However, your PR can serve as the implementation of a separate issue and will not be affected.

@pandaapo
Copy link
Contributor

Based on my current understanding of Pulsar, I have completed the review to the best of my ability. As I said in your last PR, I am not very familiar with Pulsar, so I can't guarantee the quality of my review this time. Please forgive my lack of technical expertise.

Reviewers should be responsible for the first "approved" of each PR, so please wait for further review from more members of the community.

@pandaapo pandaapo changed the title #4556 implement retry strategy based on pulsar message middleware #4556 implement retry consuming based on pulsar message middleware May 23, 2024
@pandaapo pandaapo changed the title #4556 implement retry consuming based on pulsar message middleware [ISSUE #4897] Implement retry consuming based on pulsar message middleware May 23, 2024
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.

[Enhancement] Implement retry consuming based on Pulsar message middleware
2 participants