Deprecate queue-master-locator #11565

mkuratczyk · 2024-06-25T20:51:39Z

this should not be a breaking change - all validation should still pass
CQs can now use queue-leader-locator
queue-leader-locator takes precedence over queue-master-locator if both are used
regardless of which name is used, effectively there are only two values: client-local (default) or balanced
other values (min-masters, random, least-leaders) are mapped to balanced
exclusive queues are always declared locally, as before

michaelklishin · 2024-06-26T00:19:47Z

@mkuratczyk I have added the new suite to Bazel.

ansd

Since this PR deprecates queue-master-locator, how can we in future remove support for queue-master-locator? Should queue-master-locator be placed behind a deprecated feature flag such that removal can be enforced in a future RabbitMQ version?

mkuratczyk · 2024-06-28T13:56:35Z

We can, even now, but I wouldn't rush with actually enforcing anything. The amount of code needed to support the old name is tiny and there can be applications that have that name in the codebase and we all know that for some users, application code changes are very hard (this particular setting is probably much more common in policies, that are easier to change, but still).

ansd · 2024-06-28T14:13:04Z

We can, even now, but I wouldn't rush with actually enforcing anything.

No, we can't remove support for queue-master-locator right now because - as you wrote - applications use queue-master-locator.
The question is how can we eventually remove support for queue-master-locator in RabbitMQ (e.g. version 5.0)? The deprecated feature subsystem enables "a way to declare deprecated features in the code, not only in our communication.".

Users might not read the documentation and might not even understand that queue-master-locator is deprecated.
As explained in #7390 (comment) deprecation of queue-master-locator would go through the following lifecycle:

permitted_by_default
denied_by_default
disconnected
removed

We already use this deprecated feature subsystem for various deprecated features.
It might be a good idea to also "properly" deprecate queue-master-locator using the deprecated feature subsystem.

dumbbell · 2024-06-28T14:28:44Z

As I said on Slack, I also favor the use of deprecated features because it increases the chance that the information reaches the user who rely on that feature. It also allows them to test RabbitMQ as if the feature was gone.

deps/rabbit/src/rabbit_queue_location.erl

deps/rabbit/src/rabbit_classic_queue.erl

deps/rabbit/src/rabbit_queue_location.erl

mkuratczyk · 2024-07-01T12:23:26Z

Corresponding docs PR: rabbitmq/rabbitmq-website#1961

deps/rabbit/src/rabbit_classic_queue.erl

deps/rabbit/test/classic_queue_SUITE.erl

deps/rabbit/src/rabbit_classic_queue.erl

deps/rabbit/test/classic_queue_SUITE.erl

ansd

When I want to create a classic queue in the Management UI, I only see the option Master locator although I set

deprecated_features.permit.queue_master_locator = false

in my rabbitmq.conf.

I would expect to see a Leader locator button in the Management UI.

ansd · 2024-07-11T10:53:30Z

same for policies in the Management UI:

ansd · 2024-07-11T11:15:00Z

I'm unable to test upgrades from v3.13.x to this PR branch using make run-broker because the node fails during boot with this PR branch:

2024-07-11 13:10:08.727825+02:00 [error] <0.262.0>
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0> BOOT FAILED
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0> ===========
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0> Exception during startup:
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0>
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0> throw:{timeout,{rabbitmq_metadata,rabbit@V333JFK777}}
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0>
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0>     rabbit_khepri:-register_projections/0-lc$^9/1-0-/1, line 1087
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0>     rabbit_khepri:register_projections/0, line 1088
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0>     rabbit_khepri:setup/1, line 255
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0>     rabbit:run_prelaunch_second_phase/0, line 380
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0>     rabbit:start/2, line 902
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0>     application_master:start_it_old/4, line 293
2024-07-11 13:10:08.727825+02:00 [error] <0.262.0>

ansd · 2024-07-11T11:18:15Z

If you have tested that upgrades from v3.13 to this PR branch work with classic queues with x-queue-master-locator queue argument or queue-master-locator policy set and a subsequent upgrade where the new deprecated feature is denied, and the classic queue declaration by the client keeps working without having to re-create the classic queue, I'm happy with this PR being merged.

* this should not be a breaking change - all validation should still pass * CQs can now use `queue-leader-locator` * `queue-leader-locator` takes precedence over `queue-master-locator` if both are used * regardless of which name is used, effectively there are only two values: `client-local` (default) or `balanced` * other values (`min-masters`, `random`, `least-leaders`) are mapped to `balanced` * exclusive queues are always declared locally, as before

* prevent the warning about deprecated features when they are not actually used * add a feature flag to prevent the use of queue-leader-locator until all nodes support it * don't use is_mixed_version in tests * keyfind -> keymember

Add leader-locator as an argument for all queue types Remove master-locator

* Deprecate queue-master-locator This should not be a breaking change - all validation should still pass * CQs can now use `queue-leader-locator` * `queue-leader-locator` takes precedence over `queue-master-locator` if both are used * regardless of which name is used, effectively there are only two values: `client-local` (default) or `balanced` * other values (`min-masters`, `random`, `least-leaders`) are mapped to `balanced` * Management UI no longer shows `master-locator` fields when declaring a queue/policy, but such arguments can still be used manually (unless not permitted) * exclusive queues are always declared locally, as before (cherry picked from commit f398892)

* Deprecate queue-master-locator This should not be a breaking change - all validation should still pass * CQs can now use `queue-leader-locator` * `queue-leader-locator` takes precedence over `queue-master-locator` if both are used * regardless of which name is used, effectively there are only two values: `client-local` (default) or `balanced` * other values (`min-masters`, `random`, `least-leaders`) are mapped to `balanced` * Management UI no longer shows `master-locator` fields when declaring a queue/policy, but such arguments can still be used manually (unless not permitted) * exclusive queues are always declared locally, as before (cherry picked from commit f398892) Co-authored-by: Michal Kuratczyk <michal.kuratczyk@broadcom.com>

mergify bot added bazel make labels Jun 25, 2024

mkuratczyk force-pushed the cq-leader-locator branch from e38a81a to d608ace Compare June 25, 2024 21:03

mkuratczyk force-pushed the cq-leader-locator branch 4 times, most recently from deaee1e to 7390cff Compare June 26, 2024 09:20

mkuratczyk marked this pull request as ready for review June 26, 2024 10:52

mkuratczyk requested a review from ansd June 26, 2024 10:52

ansd reviewed Jun 28, 2024

View reviewed changes

ansd requested changes Jun 28, 2024

View reviewed changes

deps/rabbit/src/rabbit_queue_location.erl Outdated Show resolved Hide resolved

deps/rabbit/src/rabbit_classic_queue.erl Show resolved Hide resolved

deps/rabbit/src/rabbit_classic_queue.erl Outdated Show resolved Hide resolved

ansd requested changes Jun 28, 2024

View reviewed changes

deps/rabbit/src/rabbit_queue_location.erl Outdated Show resolved Hide resolved

deps/rabbit/src/rabbit_queue_location.erl Outdated Show resolved Hide resolved

mkuratczyk force-pushed the cq-leader-locator branch 9 times, most recently from 692c40c to 21f03b2 Compare June 30, 2024 22:10

mkuratczyk requested a review from ansd July 1, 2024 12:24

mkuratczyk force-pushed the cq-leader-locator branch from f501367 to 60b2053 Compare July 8, 2024 07:52

ansd requested changes Jul 8, 2024

View reviewed changes

deps/rabbit/test/classic_queue_SUITE.erl Outdated Show resolved Hide resolved

deps/rabbit/test/classic_queue_SUITE.erl Outdated Show resolved Hide resolved

mkuratczyk force-pushed the cq-leader-locator branch 2 times, most recently from 3feb7fd to 3ca0d74 Compare July 9, 2024 08:23

mkuratczyk requested a review from ansd July 9, 2024 08:40

dumbbell requested changes Jul 10, 2024

View reviewed changes

deps/rabbit/src/rabbit_classic_queue.erl Show resolved Hide resolved

deps/rabbit/src/rabbit_classic_queue.erl Show resolved Hide resolved

deps/rabbit/test/classic_queue_SUITE.erl Outdated Show resolved Hide resolved

ansd requested changes Jul 11, 2024

View reviewed changes

mkuratczyk force-pushed the cq-leader-locator branch from 1d7c6b1 to 169d62a Compare July 11, 2024 16:41

michaelklishin added the backport-v4.0.x label Jul 11, 2024

mkuratczyk and others added 9 commits July 12, 2024 09:57

Make Bazel aware of unit_queue_location_SUITE, bazel run gazelle

7fd7821

Integration tests for CQ leader locator

aed298e

Remove redundant argument

44c6618

Simplify

4c0df42

Add deprecation flag for master-locator

f0537bb

Implement PR feedback

6568492

* prevent the warning about deprecated features when they are not actually used * add a feature flag to prevent the use of queue-leader-locator until all nodes support it * don't use is_mixed_version in tests * keyfind -> keymember

Try to enabled the FF in tests

a0ee907

update queues and policies mgmt pages

68e61b3

Add leader-locator as an argument for all queue types Remove master-locator

mkuratczyk force-pushed the cq-leader-locator branch from 169d62a to 68e61b3 Compare July 12, 2024 07:59

mkuratczyk merged commit f398892 into main Jul 12, 2024
194 checks passed

mkuratczyk deleted the cq-leader-locator branch July 12, 2024 11:22

mergify bot mentioned this pull request Jul 12, 2024

Deprecate queue-master-locator (backport #11565) #11699

Merged

mkuratczyk mentioned this pull request Jul 16, 2024

Deprecate queue_master_locator #10702

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate queue-master-locator #11565

Deprecate queue-master-locator #11565

mkuratczyk commented Jun 25, 2024

michaelklishin commented Jun 26, 2024

ansd left a comment

mkuratczyk commented Jun 28, 2024

ansd commented Jun 28, 2024

dumbbell commented Jun 28, 2024

mkuratczyk commented Jul 1, 2024

ansd left a comment •

edited

Loading

ansd commented Jul 11, 2024

ansd commented Jul 11, 2024

ansd commented Jul 11, 2024

Deprecate queue-master-locator #11565

Deprecate queue-master-locator #11565

Conversation

mkuratczyk commented Jun 25, 2024

michaelklishin commented Jun 26, 2024

ansd left a comment

Choose a reason for hiding this comment

mkuratczyk commented Jun 28, 2024

ansd commented Jun 28, 2024

dumbbell commented Jun 28, 2024

mkuratczyk commented Jul 1, 2024

ansd left a comment • edited Loading

Choose a reason for hiding this comment

ansd commented Jul 11, 2024

ansd commented Jul 11, 2024

ansd commented Jul 11, 2024

ansd left a comment •

edited

Loading