From aa411b2933297bc6d54bc95d8f4e4f67b4aa5335 Mon Sep 17 00:00:00 2001 From: Alexey Zatelepin Date: Thu, 16 Nov 2023 11:06:10 +0100 Subject: [PATCH] cluster: use is_cancelled_state where appropriate Previously, we determined the end state of the update with ad-hoc code each time. This is not reliable, as we can miss some state values (and indeed, the code for updating replicas revisions missed the case for force_update). Use is_cancelled_state predicate everywhere instead. (cherry picked from commit 69fd894bf89899c65d55a62517f91f5297680146) --- src/v/cluster/partition_balancer_state.cc | 2 +- src/v/cluster/scheduling/partition_allocator.cc | 11 +++-------- src/v/cluster/topic_table.cc | 10 +++------- src/v/cluster/topic_table.h | 8 ++------ src/v/cluster/topic_updates_dispatcher.cc | 11 +++-------- 5 files changed, 12 insertions(+), 30 deletions(-) diff --git a/src/v/cluster/partition_balancer_state.cc b/src/v/cluster/partition_balancer_state.cc index b1738d3e508b..4eaf8e268cac 100644 --- a/src/v/cluster/partition_balancer_state.cc +++ b/src/v/cluster/partition_balancer_state.cc @@ -119,7 +119,7 @@ partition_balancer_state::apply_snapshot(const controller_snapshot& snap) { if (auto it = topic.updates.find(p_id); it != topic.updates.end()) { const auto& update = it->second; - if (update.state == reconfiguration_state::in_progress) { + if (!is_cancelled_state(update.state)) { replicas = &update.target_assignment; } } diff --git a/src/v/cluster/scheduling/partition_allocator.cc b/src/v/cluster/scheduling/partition_allocator.cc index 65c75acd8b78..caf9357cc292 100644 --- a/src/v/cluster/scheduling/partition_allocator.cc +++ b/src/v/cluster/scheduling/partition_allocator.cc @@ -498,15 +498,10 @@ partition_allocator::apply_snapshot(const controller_snapshot& snap) { } // final counts depend on the update state - switch (update.state) { - case reconfiguration_state::in_progress: - case reconfiguration_state::force_update: - final_replicas = &update.target_assignment; - break; - case reconfiguration_state::cancelled: - case reconfiguration_state::force_cancelled: + if (is_cancelled_state(update.state)) { final_replicas = &partition.replicas; - break; + } else { + final_replicas = &update.target_assignment; } } else { final_replicas = &partition.replicas; diff --git a/src/v/cluster/topic_table.cc b/src/v/cluster/topic_table.cc index 1a1f9798daaf..118bbed1f6c4 100644 --- a/src/v/cluster/topic_table.cc +++ b/src/v/cluster/topic_table.cc @@ -321,7 +321,7 @@ topic_table::apply(finish_moving_partition_replicas_cmd cmd, model::offset o) { return ss::make_ready_future( errc::partition_not_exists); } - if (it->second.get_state() == reconfiguration_state::in_progress) { + if (!is_cancelled_state(it->second.get_state())) { // update went through and the cancellation didn't happen, we must // update replicas_revisions. p_meta_it->second.replicas_revisions = update_replicas_revisions( @@ -468,9 +468,7 @@ topic_table::apply(revert_cancel_partition_move_cmd cmd, model::offset o) { if (in_progress_it == _updates_in_progress.end()) { co_return errc::no_update_in_progress; } - if ( - in_progress_it->second.get_state() - == reconfiguration_state::in_progress) { + if (!is_cancelled_state(in_progress_it->second.get_state())) { co_return errc::no_update_in_progress; } @@ -991,9 +989,7 @@ class topic_table::snapshot_applier { update_it != topic.updates.end()) { const auto& update = update_it->second; - if ( - update.state == reconfiguration_state::in_progress - || update.state == reconfiguration_state::force_update) { + if (!is_cancelled_state(update.state)) { cur_assignment.replicas = update_it->second.target_assignment; } diff --git a/src/v/cluster/topic_table.h b/src/v/cluster/topic_table.h index 9764ab77cd06..c94bd495c47f 100644 --- a/src/v/cluster/topic_table.h +++ b/src/v/cluster/topic_table.h @@ -132,9 +132,7 @@ class topic_table { ~in_progress_update() { _probe.handle_update_finish(_previous_replicas, _target_replicas); - if ( - _state == reconfiguration_state::cancelled - || _state == reconfiguration_state::force_cancelled) { + if (is_cancelled_state(_state)) { _probe.handle_update_cancel_finish( _previous_replicas, _target_replicas); ; @@ -149,9 +147,7 @@ class topic_table { const reconfiguration_state& get_state() const { return _state; } void set_state(reconfiguration_state state, model::revision_id rev) { - if ( - _state == reconfiguration_state::in_progress - && (state == reconfiguration_state::cancelled || state == reconfiguration_state::force_cancelled)) { + if (!is_cancelled_state(_state) && is_cancelled_state(state)) { _probe.handle_update_cancel( _previous_replicas, _target_replicas); } diff --git a/src/v/cluster/topic_updates_dispatcher.cc b/src/v/cluster/topic_updates_dispatcher.cc index 6a74e9fad535..ae9205b7c6e9 100644 --- a/src/v/cluster/topic_updates_dispatcher.cc +++ b/src/v/cluster/topic_updates_dispatcher.cc @@ -459,15 +459,10 @@ topic_updates_dispatcher::collect_in_progress( continue; } const auto state = it->second.get_state(); - if (state == reconfiguration_state::in_progress) { - in_progress[p.id] = it->second.get_previous_replicas(); - } else { - vassert( - state == reconfiguration_state::cancelled - || state == reconfiguration_state::force_cancelled, - "Invalid reconfiguration state: {}", - state); + if (is_cancelled_state(state)) { in_progress[p.id] = it->second.get_target_replicas(); + } else { + in_progress[p.id] = it->second.get_previous_replicas(); } } return in_progress;