Implement PR feedback

* 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

Author: mkuratczyk

deps/rabbit/src/rabbit_classic_queue.erl

@@ -78,15 +78,21 @@
      {enables, recovery}]}).
 
 validate_policy(Args) ->
-    Strategy = proplists:get_value(<<"queue-master-locator">>, Args, unknown),
-    case rabbit_queue_location:master_locator_permitted() of
-        true ->
-            case lists:member(Strategy, rabbit_queue_location:queue_leader_locators()) of
-                true -> ok;
-                false -> {error, "~tp is not a valid master locator", [Strategy]}
-            end;
-        false ->
-            {error, "use of deprecated queue-master-locator argument is not permitted", []}
+    %% queue-master-locator was deprecated in 4.0
+    Locator = proplists:get_value(<<"queue-master-locator">>, Args, unknown),
+    case Locator of
+        unknown ->
+            ok;
+        _ ->
+            case rabbit_queue_location:master_locator_permitted() of
+                true ->
+                    case lists:member(Locator, rabbit_queue_location:queue_leader_locators()) of
+                        true -> ok;
+                        false -> {error, "~tp is not a valid master locator", [Locator]}
+                    end;
+                false ->
+                    {error, "use of deprecated queue-master-locator argument is not permitted", []}
+                end
     end.
 
 -spec is_enabled() -> boolean().
@@ -97,25 +103,24 @@ is_compatible(_, _, _) ->
     true.
 
 validate_arguments(Args) ->
-    case lists:keyfind(<<"x-queue-master-locator">>, 1, Args) of
+    case lists:keymember(<<"x-queue-master-locator">>, 1, Args) of
         false ->
             ok;
-        _ ->
+        true ->
             case rabbit_queue_location:master_locator_permitted() of
                 true ->
                     ok;
                 false ->
-                    error
+                    Warning = rabbit_deprecated_features:get_warning(
+                                queue_master_locator),
+                    {protocol_error, internal_error, "~ts", [Warning]}
             end
     end.
 
 declare(Q, Node) when ?amqqueue_is_classic(Q) ->
     case validate_arguments(amqqueue:get_arguments(Q)) of
         ok -> do_declare(Q, Node);
-        error ->
-            Warning = rabbit_deprecated_features:get_warning(
-                        queue_master_locator),
-            {protocol_error, internal_error, "~ts", [Warning]}
+        Error -> Error
     end.
 
 do_declare(Q, Node) when ?amqqueue_is_classic(Q) ->
@@ -562,11 +567,11 @@ capabilities() ->
                           <<"x-dead-letter-routing-key">>, <<"x-max-length">>,
                           <<"x-max-length-bytes">>, <<"x-max-priority">>,
                           <<"x-overflow">>, <<"x-queue-mode">>, <<"x-queue-version">>,
-                          <<"x-single-active-consumer">>, <<"x-queue-type">>,
-                          <<"x-queue-leader-locator">>] ++ case rabbit_queue_location:master_locator_permitted() of
-                                                               true -> [<<"x-queue-master-locator">>];
-                                                               false -> []
-                                                           end,
+                          <<"x-single-active-consumer">>, <<"x-queue-type">>, <<"x-queue-master-locator">>]
+                          ++ case rabbit_feature_flags:is_enabled(classic_queue_leader_locator) of
+                                 true -> [<<"x-queue-leader-locator">>];
+                                 false -> []
+                             end,
       consumer_arguments => [<<"x-priority">>, <<"x-credit">>],
       server_named => true}.
 

deps/rabbit/src/rabbit_core_ff.erl

@@ -185,3 +185,10 @@
       stability     => stable,
       depends_on    => [message_containers]
      }}).
+
+-rabbit_feature_flag(
+   {classic_queue_leader_locator,
+    #{desc          => "queue-leader-locator support in classic queues",
+      doc_url       => "https://www.rabbitmq.com/docs/clustering#replica-placement",
+      stability     => stable
+     }}).

deps/rabbit/src/rabbit_queue_location.erl

@@ -28,7 +28,7 @@
       messages =>
       #{when_permitted =>
         "queue-master-locator is deprecated. "
-        "queue-leader-locator should be used instead. Allowed values are 'client-local' and 'balanced'."}}
+        "queue-leader-locator should be used instead (allowed values are 'client-local' and 'balanced')"}}
    }).
 
 -define(QUEUE_LEADER_LOCATORS_DEPRECATED, [<<"random">>, <<"least-leaders">>, <<"min-masters">>]).

deps/rabbit/test/classic_queue_SUITE.erl

@@ -11,6 +11,10 @@
 
 -compile([nowarn_export_all, export_all]).
 
+-import(rabbit_ct_broker_helpers,
+        [get_node_config/3,
+         rpc/4,
+         rpc/5]).
 
 all() ->
     [
@@ -56,38 +60,37 @@ end_per_group(_, Config) ->
                                 rabbit_ct_client_helpers:teardown_steps() ++
                                 rabbit_ct_broker_helpers:teardown_steps()).
 
+init_per_testcase(T, Config) ->
+    case rpc(Config, rabbit_feature_flags, is_enabled, [classic_queue_leader_locator]) of
+        true ->
+            rabbit_ct_helpers:testcase_started(Config, T);
+        false ->
+            {skip, "queue-leader-locator support was added to classic queues in 4.0;"
+             "previously only queue-master-locator was allowed"}
+    end.
+
 %% -------------------------------------------------------------------
 %% Testcases.
 %% -------------------------------------------------------------------
 
 leader_locator_client_local(Config) ->
-    case rabbit_ct_helpers:is_mixed_versions() of
-        true ->
-            {skip, "x-queue-leader-locator is only supported by CQs in 4.0+"};
-        false ->
+    Servers = rabbit_ct_broker_helpers:get_node_configs(Config, nodename),
+    Q = <<"q1">>,
 
-            Servers = rabbit_ct_broker_helpers:get_node_configs(Config, nodename),
-            Q = <<"q1">>,
-
-            [begin
-                 Ch = rabbit_ct_client_helpers:open_channel(Config, Server),
-                 ?assertEqual({'queue.declare_ok', Q, 0, 0},
-                              declare(Ch, Q, [{<<"x-queue-type">>, longstr, <<"classic">>},
-                                              {<<"x-queue-leader-locator">>, longstr, <<"client-local">>}])),
-                 {ok, Leader0} = rabbit_ct_broker_helpers:rpc(Config, Server, rabbit_amqqueue, lookup, [rabbit_misc:r(<<"/">>, queue, Q)]),
-                 Leader = amqqueue:qnode(Leader0),
-                 ?assertEqual(Server, Leader),
-                 ?assertMatch(#'queue.delete_ok'{},
-                              amqp_channel:call(Ch, #'queue.delete'{queue = Q}))
-             end || Server <- Servers] end.
+    [begin
+         Ch = rabbit_ct_client_helpers:open_channel(Config, Server),
+         ?assertEqual({'queue.declare_ok', Q, 0, 0},
+                      declare(Ch, Q, [{<<"x-queue-type">>, longstr, <<"classic">>},
+                                      {<<"x-queue-leader-locator">>, longstr, <<"client-local">>}])),
+         {ok, Leader0} = rabbit_ct_broker_helpers:rpc(Config, Server, rabbit_amqqueue, lookup, [rabbit_misc:r(<<"/">>, queue, Q)]),
+         Leader = amqqueue:qnode(Leader0),
+         ?assertEqual(Server, Leader),
+         ?assertMatch(#'queue.delete_ok'{},
+                      amqp_channel:call(Ch, #'queue.delete'{queue = Q}))
+     end || Server <- Servers].
 
 leader_locator_balanced(Config) ->
-    case rabbit_ct_helpers:is_mixed_versions() of
-        true ->
-            {skip, "x-queue-leader-locator is only supported by CQs in 4.0+"};
-        false ->
-            test_leader_locator(Config, <<"x-queue-leader-locator">>, [<<"balanced">>])
-    end.
+    test_leader_locator(Config, <<"x-queue-leader-locator">>, [<<"balanced">>]).
 
 %% This test can be delted once we remove x-queue-master-locator support
 locator_deprecated(Config) ->

deps/rabbit/test/rabbitmq_4_0_deprecations_SUITE.erl

@@ -495,8 +495,7 @@ when_queue_master_locator_is_permitted_by_default(Config) ->
     ?assert(
        log_file_contains_message(
          Config, NodeA,
-         ["Deprecated features: `queue_master_locator`: queue-master-locator is deprecated. ",
-          "queue-leader-locator should be used instead."])).
+         ["Deprecated features: `queue_master_locator`: queue-master-locator is deprecated"])).
 
 when_queue_master_locator_is_not_permitted_from_conf(Config) ->
     [NodeA] = rabbit_ct_broker_helpers:get_node_configs(Config, nodename),