From ba247c0003e01fc37c8f6f77c3345e66273a4660 Mon Sep 17 00:00:00 2001 From: Alexey Zatelepin Date: Tue, 21 Nov 2023 13:58:40 +0100 Subject: [PATCH 1/2] c/topic_table: fix topic set_disabled=false idempotency check Previously we incorrectly assumed that fully enabling a partially disabled topic is a no-op. This is a no-op is if the topic was previously fully enabled. --- src/v/cluster/topic_table.cc | 4 ++-- src/v/cluster/topic_table.h | 12 ++++++++++-- src/v/cluster/topics_frontend.cc | 6 +++++- src/v/cluster/types.h | 6 +++--- src/v/redpanda/admin/server.cc | 4 ++-- 5 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/v/cluster/topic_table.cc b/src/v/cluster/topic_table.cc index 18328e937ebc6..81dac2da09286 100644 --- a/src/v/cluster/topic_table.cc +++ b/src/v/cluster/topic_table.cc @@ -644,12 +644,12 @@ topic_table::apply(set_topic_partitions_disabled_cmd cmd, model::offset o) { disabled_set.remove(*cmd.value.partition_id, assignments); } - if (disabled_set.is_empty()) { + if (disabled_set.is_fully_enabled()) { _disabled_partitions.erase(disabled_it); } } else { if (cmd.value.disabled) { - _disabled_partitions[cmd.value.ns_tp].set_topic_disabled(); + _disabled_partitions[cmd.value.ns_tp].set_fully_disabled(); } else { _disabled_partitions.erase(cmd.value.ns_tp); } diff --git a/src/v/cluster/topic_table.h b/src/v/cluster/topic_table.h index 49e78275854e6..ec3d4f486ed7c 100644 --- a/src/v/cluster/topic_table.h +++ b/src/v/cluster/topic_table.h @@ -518,12 +518,20 @@ class topic_table { return &it->second; } - bool is_disabled(model::topic_namespace_view ns_tp) const { + bool is_fully_disabled(model::topic_namespace_view ns_tp) const { auto it = _disabled_partitions.find(ns_tp); if (it == _disabled_partitions.end()) { return false; } - return it->second.is_topic_disabled(); + return it->second.is_fully_disabled(); + } + + bool is_fully_enabled(model::topic_namespace_view ns_tp) const { + auto it = _disabled_partitions.find(ns_tp); + if (it == _disabled_partitions.end()) { + return true; + } + return it->second.is_fully_enabled(); } bool is_disabled( diff --git a/src/v/cluster/topics_frontend.cc b/src/v/cluster/topics_frontend.cc index d6d151028a5b4..e1d97a06e100f 100644 --- a/src/v/cluster/topics_frontend.cc +++ b/src/v/cluster/topics_frontend.cc @@ -1040,7 +1040,11 @@ ss::future topics_frontend::set_topic_partitions_disabled( if (!_topics.local().contains(ns_tp)) { co_return errc::topic_not_exists; } - if (_topics.local().is_disabled(ns_tp) == disabled) { + if (disabled && _topics.local().is_fully_disabled(ns_tp)) { + // no-op + co_return errc::success; + } + if (!disabled && _topics.local().is_fully_enabled(ns_tp)) { // no-op co_return errc::success; } diff --git a/src/v/cluster/types.h b/src/v/cluster/types.h index d35b55a0dba25..62ecfbeed8ec4 100644 --- a/src/v/cluster/types.h +++ b/src/v/cluster/types.h @@ -4418,12 +4418,12 @@ struct topic_disabled_partitions_set bool is_disabled(model::partition_id id) const { return !partitions || partitions->contains(id); } - bool is_topic_disabled() const { return !partitions.has_value(); } - bool is_empty() const { return partitions && partitions->empty(); } + bool is_fully_disabled() const { return !partitions.has_value(); } + bool is_fully_enabled() const { return partitions && partitions->empty(); } void add(model::partition_id id); void remove(model::partition_id id, const assignments_set& all_partitions); - void set_topic_disabled() { partitions = std::nullopt; } + void set_fully_disabled() { partitions = std::nullopt; } }; } // namespace cluster diff --git a/src/v/redpanda/admin/server.cc b/src/v/redpanda/admin/server.cc index 2cec6d19ab8cd..f0fb4803b906a 100644 --- a/src/v/redpanda/admin/server.cc +++ b/src/v/redpanda/admin/server.cc @@ -4530,13 +4530,13 @@ fragmented_vector topic2cluster_partitions( // fast exits if ( disabled_filter.value() - && (!disabled_set || disabled_set->is_empty())) { + && (!disabled_set || disabled_set->is_fully_enabled())) { return ret; } if ( !disabled_filter.value() && disabled_set - && disabled_set->is_topic_disabled()) { + && disabled_set->is_fully_disabled()) { return ret; } } From 37dd96c0570288af4c343242b221c9aeadeb76a8 Mon Sep 17 00:00:00 2001 From: Alexey Zatelepin Date: Tue, 21 Nov 2023 14:02:00 +0100 Subject: [PATCH 2/2] tests: add test for fully enabling a partially disabled topic --- tests/rptest/tests/recovery_mode_test.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/rptest/tests/recovery_mode_test.py b/tests/rptest/tests/recovery_mode_test.py index 33144fa5520e8..187bee85bb70e 100644 --- a/tests/rptest/tests/recovery_mode_test.py +++ b/tests/rptest/tests/recovery_mode_test.py @@ -242,7 +242,7 @@ def test_apis(self): rpk = RpkTool(self.redpanda) admin = Admin(self.redpanda) - topics = ["mytopic1", "mytopic2", "mytopic3"] + topics = ["mytopic1", "mytopic2", "mytopic3", "mytopic4"] for topic in topics: rpk.create_topic(topic, partitions=3, replicas=3) @@ -263,6 +263,13 @@ def test_apis(self): partition=1, value=False) + admin.set_partitions_disabled(ns="kafka", + topic="mytopic4", + partition=0) + admin.set_partitions_disabled(ns="kafka", + topic="mytopic4", + value=False) + self.sync() def pi(topic_partition, disabled=False): @@ -279,6 +286,9 @@ def pi(topic_partition, disabled=False): pi('mytopic3/0', True), pi('mytopic3/1', False), pi('mytopic3/2', True), + pi('mytopic4/0', False), + pi('mytopic4/1', False), + pi('mytopic4/2', False), ] def filtered(topic, partition, disabled):