Add an option to dead-letter rejected publishes #2015

Invizory · 2019-05-23T18:09:59Z

Proposed Changes

Add reject-publish-dlx overflow strategy, which is similar to reject-publish strategy, but also dead-letters rejected messages.

Closes #1443

Types of Changes

Bug fix (non-breaking change which fixes issue #NNNN)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause an observable behavior change in existing systems)
Documentation improvements (corrections, new content, etc)
Cosmetic change (whitespace, formatting, etc)

Checklist

I have read the CONTRIBUTING.md document
I have signed the CLA (see https://cla.pivotal.io/sign/rabbitmq)
All tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
Any dependent changes have been merged and published in related repositories

Further Comments

TODOs:

Add tests that cover the same behaviour covered by existing tests for reject-publish.
Documentation should also be updated accordingly, i.e. https://github.com/rabbitmq/rabbitmq-website/blob/master/site/maxlength.xml. I'm ready to propose a corresponding pull request, if this is going to be approved.

Invizory · 2019-05-24T14:31:44Z

It looks like confirms_rejects_SUITE tests are unstable and also fail in some test runs in master.

Here is the possible outputs of make ct-confirms_rejects_SUITE in both master and this branch:

TEST INFO: 1 test(s), 4 case(s) in 1 suite(s)

== confirms_rejects_SUITE ==

  * [parallel_tests]
    confirms_rejects_conflict (00:23.513)
    mixed_dead_alive_queues_reject (00:11.320)
    dead_queue_rejects (00:01.250)
    policy_resets_to_default (00:04.270)


confirms_rejects_SUITE > parallel_tests > mixed_dead_alive_queues_reject
    #1. {error,
            {timeout_waiting_for_ack,
                [{confirms_rejects_SUITE,mixed_dead_alive_queues_reject,1,
                     [{file,"test/confirms_rejects_SUITE.erl"},{line,176}]},
                 {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1748}]},
                 {test_server,run_test_case_eval1,6,
                     [{file,"test_server.erl"},{line,1263}]},
                 {test_server,run_test_case_eval,9,
                     [{file,"test_server.erl"},{line,1195}]}]}}

TEST INFO: 1 test(s), 4 case(s) in 1 suite(s)

== confirms_rejects_SUITE ==

  * [parallel_tests]
    confirms_rejects_conflict (00:22.307)
    policy_resets_to_default (00:04.322)
    dead_queue_rejects (00:01.218)
    mixed_dead_alive_queues_reject (00:01.331)

hairyhum · 2019-05-27T14:58:22Z

@Invizory hi, the code looks good. I'm going to look at the test case. Can you also add this behaviour to confirms_rejects_SUITE to expect the same behaviour as reject-publish behaviour?

hairyhum · 2019-05-27T15:25:35Z

I've increased some timeouts in the master branch. Can you try the mixed_dead_alive_queues_reject test again?
The failure may depend on the environment performance and I can't get it on my machine, but we had it in our CI pipeline once or twice.

Invizory · 2019-05-27T15:27:11Z

Can you also add this behaviour to confirms_rejects_SUITE to expect the same behaviour as reject-publish behaviour?

@hairyhum, sure, that's a good idea.

Invizory · 2019-05-27T16:19:17Z

I've increased some timeouts in the master branch. Can you try the mixed_dead_alive_queues_reject test again?

@hairyhum, at the first glance that seems to fix the problem, however, after a few runs in the master branch, I've got the following:

TEST INFO: 1 test(s), 4 case(s) in 1 suite(s)

== confirms_rejects_SUITE ==

  * [parallel_tests]
    dead_queue_rejects (00:01.278)
    confirms_rejects_conflict (00:25.049)
    policy_resets_to_default (00:04.337)
    mixed_dead_alive_queues_reject (00:51.383)


confirms_rejects_SUITE > parallel_tests > mixed_dead_alive_queues_reject
    #1. {error,
            {timeout_waiting_for_ack,
                [{confirms_rejects_SUITE,mixed_dead_alive_queues_reject,1,
                     [{file,"test/confirms_rejects_SUITE.erl"},{line,177}]},
                 {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1748}]},
                 {test_server,run_test_case_eval1,6,
                     [{file,"test_server.erl"},{line,1263}]},
                 {test_server,run_test_case_eval,9,
                     [{file,"test_server.erl"},{line,1195}]}]}}

I've also played with timeouts before and it doesn't seem to help.

Update: Also tested locally on fcae736, still unstable:

TEST INFO: 1 test(s), 4 case(s) in 1 suite(s)

== confirms_rejects_SUITE ==

  * [parallel_tests]
    confirms_rejects_conflict (00:26.216)
    dead_queue_rejects (00:01.305)
    mixed_dead_alive_queues_reject (00:51.343)
    policy_resets_to_default (00:04.323)


confirms_rejects_SUITE > parallel_tests > mixed_dead_alive_queues_reject
    #1. {error,
            {{timeout_waiting_for_nack,{messages,[]}},
             [{confirms_rejects_SUITE,mixed_dead_alive_queues_reject,1,
                  [{file,"test/confirms_rejects_SUITE.erl"},{line,177}]},
              {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1748}]},
              {test_server,run_test_case_eval1,6,
                  [{file,"test_server.erl"},{line,1263}]},
              {test_server,run_test_case_eval,9,
                  [{file,"test_server.erl"},{line,1195}]}]}}

Invizory · 2019-05-27T16:36:39Z

The rebased branch is also failing on CI, but on another test and for another reason:

confirms_rejects_SUITE:dead_queue_rejects failed on line 124
Reason: expecting_nack_got_ack

I have failed to reproduce the same behaviour locally.

Add `reject-publish-dlx` overflow strategy, which is similar to `reject-publish` strategy, but also dead-letters rejected messages. Closes rabbitmq#1443

michaelklishin · 2019-05-27T19:58:30Z

The test in question tries to kill the queue process in order to simulate a nack. That requires pretty good timing as queues will be restarted when they run into an exception, so there is a natural race condition in this test. I see no good way to address this without changing the supervision tree/implementation.

Let's focus on the rest of the suite and test coverage for this new behavior.

michaelklishin · 2019-05-27T20:02:09Z

I guess we can alter the test to run a number of iterations and expect at least some nacks. That'd increase the probability of it passing but that's about it.

hairyhum · 2019-05-27T20:33:55Z

There is another commit in master which makes the test try harder to kill the queue. But I agree that it's not related to this PR.

This change splits the tests into groups for both the existing `reject-publish` overflow strategy and the new `reject-publish-dlx`. The corresponding queue names have been suffixed to make the tests in the groups independent and thus parallel. See rabbitmq#1443

Invizory · 2019-05-28T00:03:17Z

Can you also add this behaviour to confirms_rejects_SUITE to expect the same behaviour as reject-publish behaviour?

@hairyhum, done in f145c18.

Add tests for `reject-publish-dlx` overflow strategy for all corresponding tests for `reject-publish` strategy. The tests are grouped to avoid code duplication. See rabbitmq#1443

Invizory · 2019-05-28T01:12:28Z

I’ve also added tests that cover the same behaviour covered by existing tests for reject-publish.

hairyhum

Looks good.

hairyhum · 2019-05-28T13:42:43Z

This also needs doc update in https://github.com/rabbitmq/rabbitmq-website

See rabbitmq/rabbitmq-server#1443, rabbitmq/rabbitmq-server#2015

Invizory · 2019-05-28T19:13:20Z

This also needs doc update in rabbitmq/rabbitmq-website

@hairyhum, done in rabbitmq/rabbitmq-website#774.

hairyhum · 2019-05-29T13:27:01Z

Management help message rabbitmq/rabbitmq-management@1d4be7f

michaelklishin · 2019-05-29T13:38:12Z

@Invizory thank you for contributing this!

…d in rabbitmq/rabbitmq-server#2015

…n as defined in rabbitmq/rabbitmq-server#2015

See rabbitmq/rabbitmq-server#1443, rabbitmq/rabbitmq-server#2015

Invizory force-pushed the dead-letter-rejected-published branch 4 times, most recently from 3a5d535 to 73f59b9 Compare May 23, 2019 19:24

Invizory force-pushed the dead-letter-rejected-published branch from 73f59b9 to bc82718 Compare May 27, 2019 15:38

Add an option to dead-letter rejected publishes

89f464f

Add `reject-publish-dlx` overflow strategy, which is similar to `reject-publish` strategy, but also dead-letters rejected messages. Closes rabbitmq#1443

Invizory force-pushed the dead-letter-rejected-published branch from bc82718 to 89f464f Compare May 27, 2019 19:57

Invizory force-pushed the dead-letter-rejected-published branch from 3a7d09f to f145c18 Compare May 27, 2019 23:18

Add remaining tests for reject-publish-dlx

c3d9fc5

Add tests for `reject-publish-dlx` overflow strategy for all corresponding tests for `reject-publish` strategy. The tests are grouped to avoid code duplication. See rabbitmq#1443

hairyhum approved these changes May 28, 2019

View reviewed changes

Invizory added a commit to Invizory/rabbitmq-website that referenced this pull request May 28, 2019

Document reject-publish-dlx overflow behaviour

2b575c5

See rabbitmq/rabbitmq-server#1443, rabbitmq/rabbitmq-server#2015

Invizory mentioned this pull request May 28, 2019

Document an option to dead-letter rejected publishes rabbitmq/rabbitmq-website#774

Merged

hairyhum merged commit aeb23f7 into rabbitmq:master May 29, 2019

RedMu added a commit to RedMu/rabbitmq-mock that referenced this pull request Jul 8, 2020

Introduced behaviour for reject-publish-dlx overflow option as define…

d87f10b

…d in rabbitmq/rabbitmq-server#2015

RedMu added a commit to RedMu/rabbitmq-mock that referenced this pull request Jul 9, 2020

fridujo#9 Introduced behaviour for reject-publish-dlx overflow optio…

e772667

…n as defined in rabbitmq/rabbitmq-server#2015

Invizory added a commit to Invizory/rabbitmq-website that referenced this pull request Jun 27, 2021

Restore documentation on reject-publish-dlx

9206fcc

See rabbitmq/rabbitmq-server#1443, rabbitmq/rabbitmq-server#2015

Invizory mentioned this pull request Jun 27, 2021

Restore documentation on reject-publish-dlx overflow setting rabbitmq/rabbitmq-website#1218

Merged

michaelklishin pushed a commit to rabbitmq/rabbitmq-website that referenced this pull request Aug 9, 2021

Restore documentation on reject-publish-dlx

4301b17

See rabbitmq/rabbitmq-server#1443, rabbitmq/rabbitmq-server#2015

Add an option to dead-letter rejected publishes #2015

Add an option to dead-letter rejected publishes #2015

Uh oh!

Conversation

Invizory commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Types of Changes

Checklist

Further Comments

Uh oh!

Invizory commented May 24, 2019

Uh oh!

hairyhum commented May 27, 2019

Uh oh!

hairyhum commented May 27, 2019

Uh oh!

Invizory commented May 27, 2019

Uh oh!

Invizory commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Invizory commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelklishin commented May 27, 2019

Uh oh!

michaelklishin commented May 27, 2019

Uh oh!

hairyhum commented May 27, 2019

Uh oh!

Invizory commented May 28, 2019

Uh oh!

Invizory commented May 28, 2019

Uh oh!

hairyhum left a comment

Choose a reason for hiding this comment

Uh oh!

hairyhum commented May 28, 2019

Uh oh!

Invizory commented May 28, 2019

Uh oh!

hairyhum commented May 29, 2019

Uh oh!

michaelklishin commented May 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Invizory commented May 23, 2019 •

edited

Loading

Invizory commented May 27, 2019 •

edited

Loading

Invizory commented May 27, 2019 •

edited

Loading