Merge pull request #15105 from ztlpn/v23.2.x-bp

[v23.2.x] c/topic_table: fix force_update replicas_revisions update
redpanda-data · Nov 23, 2023 · 98c5d08 · 98c5d08
2 parents ae9a71b + aa411b2
commit 98c5d08
Show file tree

Hide file tree

Showing 6 changed files with 25 additions and 30 deletions.
diff --git 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
@@ -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
@@ -321,7 +321,7 @@ topic_table::apply(finish_moving_partition_replicas_cmd cmd, model::offset o) {
         return ss::make_ready_future<std::error_code>(
           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
@@ -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
@@ -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;

diff --git a/src/v/cluster/types.h b/src/v/cluster/types.h
@@ -3805,6 +3805,19 @@ enum class reconfiguration_state {
     force_cancelled = 3
 };
 
+inline bool is_cancelled_state(reconfiguration_state rs) {
+    switch (rs) {
+    case reconfiguration_state::in_progress:
+    case reconfiguration_state::force_update:
+        return false;
+    case reconfiguration_state::cancelled:
+    case reconfiguration_state::force_cancelled:
+        return true;
+    default:
+        __builtin_unreachable();
+    }
+}
+
 std::ostream& operator<<(std::ostream&, reconfiguration_state);
 
 struct replica_bytes {