c/leader_balancer: remove unused recommended_reassignment() method

Random strategy doesn't use it and instead goes through the list of all
possible reassignments in random order.

src/v/cluster/scheduling/leader_balancer_constraints.cc

@@ -46,15 +46,6 @@ void even_topic_distribution_constraint::update_index(const reassignment& r) {
     _topic_shard_index.at(topic_id).at(r.to) += 1;
 }
 
-std::optional<reassignment>
-even_topic_distribution_constraint::recommended_reassignment() {
-    // This method is deprecated and is ony used in `leader_balancer_greedy`
-    // which doesn't use the `even_topic_distributon_constraint`. Hence there is
-    // no need to implement it here. Once the greedy balancer has been removed
-    // this should be removed as well.
-    vassert(false, "not implemented");
-}
-
 void even_topic_distribution_constraint::rebuild_indexes() {
     _topic_shard_index.clear();
     _topic_replica_index.clear();
@@ -153,64 +144,6 @@ double even_topic_distribution_constraint::adjusted_error(
     return current_error;
 }
 
-std::optional<reassignment>
-even_shard_load_constraint::recommended_reassignment() {
-    auto [load, load_map] = build_load_indexes();
-    const auto curr_error = error();
-
-    // Consider each group from high load core, and record the reassignment
-    // involving the lowest load "to" core.
-    for (const auto& from : boost::adaptors::reverse(load)) {
-        if (mi().muted_nodes().contains(from->first.node_id)) {
-            continue;
-        }
-
-        constexpr size_t load_unset = std::numeric_limits<size_t>::max();
-        size_t lowest_load = load_unset;
-        reassignment lowest_reassign{};
-
-        // Consider each group from high load core, and record the
-        // reassignment involving the lowest load "to" core.
-        for (const auto& group : from->second) {
-            if (mi().muted_groups().contains(
-                  static_cast<uint64_t>(group.first))) {
-                continue;
-            }
-
-            // iterate over all the replicas and look for the lowest load
-            // shard in the replica list
-            for (const auto& to_shard : group.second) {
-                auto load = load_map.at(to_shard);
-                if (likely(load >= lowest_load)) {
-                    // there is no point in evaluating this move, it is
-                    // worse than the best one we've found so far.
-                    continue;
-                }
-
-                if (mi().muted_nodes().contains(to_shard.node_id)) {
-                    continue;
-                }
-
-                lowest_load = load;
-                lowest_reassign = {group.first, from->first, to_shard};
-            }
-        }
-
-        if (lowest_load != load_unset) {
-            // We found a possible reassignment while looking at the current
-            // "from" shard, and while it is the best possible reassignment
-            // found it may not improve the error
-            auto new_error = adjusted_error(
-              curr_error, lowest_reassign.from, lowest_reassign.to);
-            if (new_error + error_jitter < curr_error) {
-                return lowest_reassign;
-            }
-        }
-    }
-
-    return std::nullopt;
-}
-
 double even_shard_load_constraint::adjusted_error(
   double current_error,
   const model::broker_shard& from,
@@ -320,12 +253,4 @@ double pinning_constraint::evaluate_internal(const reassignment& r) {
     return diff;
 }
 
-std::optional<reassignment> pinning_constraint::recommended_reassignment() {
-    // This method is deprecated and is ony used in `leader_balancer_greedy`
-    // which doesn't use the `even_topic_distributon_constraint`. Hence there is
-    // no need to implement it here. Once the greedy balancer has been removed
-    // this should be removed as well.
-    vassert(false, "not implemented");
-}
-
 } // namespace cluster::leader_balancer_types

src/v/cluster/scheduling/leader_balancer_constraints.h

@@ -133,20 +133,6 @@ class even_topic_distribution_constraint final
 
     void update_index(const reassignment& r) override;
 
-    /*
-     * Uses a greedy algorithm to generate a recommended reassignment
-     * that will reduce error for this constraint.
-     *
-     * The gist is that it searches topics in order of how skewed they are.
-     * Then for each topic it finds a partiton on the node with the most
-     * leadership and tries to move it to one of its replica nodes. If this
-     * reassignment reduces error its returned.
-     *
-     * If no reassignment can reduce error for this constraint std::nullopt is
-     * returned.
-     */
-    std::optional<reassignment> recommended_reassignment() override;
-
 private:
     std::reference_wrapper<const shard_index> _si;
     std::reference_wrapper<const muted_index> _mi;
@@ -233,23 +219,6 @@ class even_shard_load_constraint final
         _error = adjusted_error(_error, r.from, r.to);
     }
 
-    /*
-     * Find a group reassignment that improves overall error. The general
-     * approach is to select a group from the highest loaded shard and move
-     * leadership for that group to the least loaded shard that the group is
-     * compatible with.
-     *
-     * Clearly this is a costly method in terms of runtime complexity.
-     * Measurements for clusters with several thousand partitions indicate a
-     * real time execution cost of at most a couple hundred micros. Other
-     * strategies are sure to improve this as we tackle larger configurations.
-     *
-     * Muted nodes are nodes that should be treated as if they have no available
-     * capacity. So do not move leadership to a muted node, but any leaders on a
-     * muted node should not be touched in case the mute is temporary.
-     */
-    std::optional<reassignment> recommended_reassignment() override;
-
     std::vector<shard_load> stats() const;
 
     double calc_target_load() const;
@@ -314,8 +283,6 @@ class pinning_constraint final : public soft_constraint {
 private:
     double evaluate_internal(const reassignment& r) override;
 
-    std::optional<reassignment> recommended_reassignment() override;
-
 private:
     std::reference_wrapper<const group_id_to_topic_id> _group2topic;
     preference_index _preference_idx;

src/v/cluster/scheduling/leader_balancer_types.h

@@ -97,8 +97,6 @@ class soft_constraint {
         auto ret = evaluate_internal(r);
         return ret;
     }
-
-    virtual std::optional<reassignment> recommended_reassignment() = 0;
 };
 
 } // namespace cluster::leader_balancer_types

src/v/cluster/tests/leader_balancer_constraints_test.cc

@@ -228,7 +228,6 @@ BOOST_AUTO_TEST_CASE(even_topic_no_error_even_shard_error) {
       std::move(g_id_to_t_id), shard_index, muted_index);
 
     BOOST_REQUIRE(even_shard_con.error() > 0);
-    BOOST_REQUIRE(even_shard_con.recommended_reassignment().has_value());
 }
 
 #include "cluster/scheduling/leader_balancer_random.h"