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

Add poison queues support for RabbitMQ #648

Merged
merged 37 commits into from
Oct 6, 2024

Conversation

marcominerva
Copy link
Contributor

Motivation and Context (Why the change? What's the scenario?)

See #408.

Copy link
Collaborator

@dluc dluc left a comment

Choose a reason for hiding this comment

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

a couple of minor changes. I haven't tested/fully reviewed the logic yet

extensions/RabbitMQ/RabbitMqConfig.cs Outdated Show resolved Hide resolved
extensions/RabbitMQ/RabbitMqConfig.cs Outdated Show resolved Hide resolved
@marcominerva marcominerva requested a review from dluc June 6, 2024 06:56
@dluc dluc added waiting for author Waiting for author to reply or address comments work in progress labels Jun 9, 2024
@marcominerva
Copy link
Contributor Author

marcominerva commented Jun 11, 2024

I have updated this PR using Quorum Queues and Dead Letter Queues.

There are a couple of issues:

  1. After creating the Quorum Queue:
this._channel.QueueDeclare(
    queue: this._queueName,
    durable: true,
    exclusive: false,
    autoDelete: false,
    arguments: new Dictionary<string, object>
    {
        ["x-queue-type"] = "quorum",
        ["x-delivery-limit"] = this._config.MaxRetriesBeforePoisonQueue,
        ["x-dead-letter-exchange"] = poisonExchange
    });

The delivery limit cannot be changed, otherwise an exception will be thrown. So, we cannot modify the value of MaxRetriesBeforePoisonQueue. To make this value dynamic, we should use a policy, but it seems that policies aren't supported by the RabbitMQ .NET client (the only way to configure them is making an HTTP call to RabbitMQ API).

  1. Messages are requeued instantly, because there isn't a native way to have a delayed retry.

@marcominerva marcominerva requested a review from dluc June 11, 2024 12:44
@marcominerva
Copy link
Contributor Author

Hi @dluc!
Have you got the chance to think about the issues I mentioned? Do you have any idea about how to handle them?

@dluc
Copy link
Collaborator

dluc commented Jun 21, 2024

Hi @dluc! Have you got the chance to think about the issues I mentioned? Do you have any idea about how to handle them?

not yet sorry, other projects keeping me busy. I don't know when I'll be able to look through this - I'm hoping to merge the streaming PRs first, to improve the end user experience

@marcominerva
Copy link
Contributor Author

marcominerva commented Jul 9, 2024

Hi @dluc,

Given that it is not possible to change the x-delivery-limit value after queue creation, I have added a try... catch block to log the error and throw an exception that explains the problem:

https://github.com/marcominerva/kernel-memory/blob/c2f2c631757cca5f1ee093a40239968b6eb4b76d/extensions/RabbitMQ/RabbitMQPipeline.cs#L74-L92

Let me know if it could be a solution.

@marcominerva
Copy link
Contributor Author

@dluc, GitHub signals that this PR has 1 change requested, but it seems that all the request changes have been addressed, or am I missing something?

extensions/RabbitMQ/RabbitMQPipeline.cs Outdated Show resolved Hide resolved
extensions/RabbitMQ/RabbitMQPipeline.cs Outdated Show resolved Hide resolved
extensions/RabbitMQ/RabbitMQPipeline.cs Outdated Show resolved Hide resolved
@dluc
Copy link
Collaborator

dluc commented Oct 6, 2024

@marcominerva I think the PR is ready, sorry about the long delay. About the issue with max retries, I think we can live with it for now. The only option I see would be removing "x-delivery-limit" and handling the count manually, checking the "x-delivery-count" header. It would be easy, but let's see if anyone complains about this first.
I've made a few changes:

  • log the number of attempts
  • log an error on the last attempt
  • enable the consumer only after all queues and exchange are setup
  • change exchange suffix, make it shorter to allow longer queue names
  • some more validations
  • some more logging
  • added a test application showing messaging coming out of the dead letter queue

@dluc dluc merged commit 759cc43 into microsoft:main Oct 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to reply or address comments work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants