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

Bugfix: DLX parameters aren't updated when the messages is expired #601

Merged
merged 6 commits into from
Dec 12, 2023

Conversation

snichme
Copy link
Member

@snichme snichme commented Nov 27, 2023

WHAT is this pull request doing?

The effect of this bug is that count on a DLX message is counted up to 2 but never higher. It's set to 1 when the headers are created, we get the first count and increase that, but instead of updating in place the new count is added as a new field in the table.

AMP::Table doesn't support Symbol for it's #delete method, which results in when using #merge! the new pairs are added onto the backing IO for each call.

The result is that after three expires the table looks like this:

AMQ::Protocol::Table(@queue="alarms.notifications", @reason="rejected", @exchange="amq.topic", @count=1, @time=2023-11-27 12:46:51.0 UTC, @routing-keys=["alarms.notify"], @count=2, @time=2023-11-27 12:47:01.0 UTC, @routing-keys=["alarms.notify"], @count=2, @time=2023-11-27 12:47:11.0 UTC, @routing-keys=["alarms.notify"])

If instead each key is set it overrides the previous value as expected.

HOW can this pull request be tested?

Not sure how to write specs for this

It can be tested manually by publish a message to a queue that is setup to when the message is rejected, it ends up in a DLX queue. Setup a TTL on the DLX queue so that when the TTL expires the messages is published back to the original queue, when that happens, look at the messages headers x-death

@snichme snichme requested review from spuun and kickster97 November 27, 2023 13:05
@spuun
Copy link
Member

spuun commented Nov 27, 2023

I can try to create a spec or two for this.

I've working on fix for amq-protocol here: cloudamqp/amq-protocol.cr#16

Copy link
Member

@viktorerlingsson viktorerlingsson left a comment

Choose a reason for hiding this comment

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

LGTM

snichme and others added 4 commits December 12, 2023 11:53
The effect of this bug is that `count` on a DLX message is counted up to 2 but never higher.
It's set to 1 when the headers are created, we get the frist count and increase that but instead of
updating in place the new count is added as a new field in the table.

AMP::Table doesn't support `Symbol` for it's `#delete` method which results in when using `#merge!`
the new pairs are added onto the backing IO for each call.

The result is that after three expires the table looks like this:
```
AMQ::Protocol::Table(@Queue="alarms.notifications", @Reason="rejected", @eXchange="amq.topic", @count=1, @time=2023-11-27 12:46:51.0 UTC, @routing-keys=["alarms.notify"], @count=2, @time=2023-11-27 12:47:01.0 UTC, @routing-keys=["alarms.notify"], @count=2, @time=2023-11-27 12:47:11.0 UTC, @routing-keys=["alarms.notify"])
```

If instead each key is set it overrides the previous value as expected.
@kickster97 kickster97 force-pushed the bugfix-dlx-parameters-not-updated branch from d1c573c to b08dfeb Compare December 12, 2023 10:56
@kickster97 kickster97 merged commit 232fd6b into main Dec 12, 2023
13 of 18 checks passed
@kickster97 kickster97 deleted the bugfix-dlx-parameters-not-updated branch December 12, 2023 11:20
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.

4 participants