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.

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

Conversation

ztlpn
Copy link
Contributor

@ztlpn ztlpn commented Nov 21, 2023

Previously we incorrectly assumed that fully enabling a partially disabled topic is a no-op. This is a no-op only if the topic was previously fully enabled.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.2.x
  • v23.1.x
  • v22.3.x

Release Notes

  • none

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.
@ztlpn ztlpn force-pushed the fix-whole-topic-enable branch from a9fb5f9 to 37dd96c Compare November 21, 2023 13:50
@vbotbuildovich
Copy link
Collaborator

@ztlpn ztlpn changed the title Fix enabling whole topic idempotency check Fix enabling whole topic no-op check Nov 22, 2023
Copy link
Contributor

@bharathv bharathv left a comment

Choose a reason for hiding this comment

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

lgtm.

@@ -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.

@ztlpn ztlpn merged commit b58d8bb into redpanda-data:dev Nov 22, 2023
22 checks passed
@ztlpn ztlpn deleted the fix-whole-topic-enable branch November 27, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants