Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix enabling whole topic no-op check #15071

Merged
merged 2 commits into from
Nov 22, 2023
Merged

Fix enabling whole topic no-op check #15071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba247c0
c/topic_table: fix topic set_disabled=false idempotency check
ztlpn Nov 21, 2023
37dd96c
tests: add test for fully enabling a partially disabled topic
ztlpn Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/v/cluster/topic_table.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
12 changes: 10 additions & 2 deletions src/v/cluster/topic_table.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
6 changes: 5 additions & 1 deletion src/v/cluster/topics_frontend.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1040,7 +1040,11 @@ ss::future<std::error_code> 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;
}
Expand Down
6 changes: 3 additions & 3 deletions src/v/cluster/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a general question, for my understanding, is there a difference between partitions=std::nullopt and partitions.empty() = true? Curious if we really need an optional here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is explained in the comment a few lines earlier - partitions=nullopt means all partitions are disabled. It looks a bit awkward but I thought that this is better than having a separate "all disabled" flag.

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
Expand Down
4 changes: 2 additions & 2 deletions src/v/redpanda/admin/server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4530,13 +4530,13 @@ fragmented_vector<cluster_partition_info> 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;
}
}
Expand Down
12 changes: 11 additions & 1 deletion tests/rptest/tests/recovery_mode_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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):
Expand All @@ -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):
Expand Down