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

Topic-aware replica placement in partition_allocator::reallocate_partition #17704

Merged

Conversation

ztlpn
Copy link
Contributor

@ztlpn ztlpn commented Apr 8, 2024

Add topic awareness to partition_allocator::reallocate_partition and enable it in the usage sites of this API. Additionally, update health_manager to use topics_frontend::do_update_topic_properties instead of dispatching partition reconfigurations manually.

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.3.x
  • v23.2.x

Release Notes

  • none

@ztlpn ztlpn force-pushed the partition-balancer-topic-aware-followup branch from 851ad6d to 3747e15 Compare April 8, 2024 22:11
Do all reallocations in a single invoke_on call instead of doing a
cross-shard call for each partition. No semantic change.
This is better as the whole topic is updated by one controller command,
preventing the situation where different partitions of a topic have
different replication factors. Also, we get topic awareness of new
replica placement for free.
@ztlpn ztlpn force-pushed the partition-balancer-topic-aware-followup branch from 3747e15 to 2868a6c Compare April 9, 2024 11:49
@@ -321,7 +321,8 @@ result<allocated_partition> partition_allocator::reallocate_partition(
partition_constraints p_constraints,
const partition_assignment& current_assignment,
const partition_allocation_domain domain,
const std::vector<model::node_id>& replicas_to_reallocate) {
const std::vector<model::node_id>& replicas_to_reallocate,
node2count_t* existing_replica_counts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I was debating pointer vs an optional<reference_wrapper<node2count_t>>.. the latter looks really ugly I guess, sigh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I always feel a bit guilty about using raw pointers... OTOH I am always drawn to it by its simplicity and versatility :)

Copy link
Member

Choose a reason for hiding this comment

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

@bharathv @ztlpn https://github.com/redpanda-data/redpanda/blob/dev/src/v/net/transport.h#L111 oh I saw this pattern being discussed on slack and tried it. passed code review :P

@ztlpn ztlpn merged commit 1990bd6 into redpanda-data:dev Apr 10, 2024
17 checks passed
@ztlpn ztlpn deleted the partition-balancer-topic-aware-followup branch April 10, 2024 11:47
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.

None yet

4 participants