Skip to content

Conversation

@bmckerry
Copy link
Member

@bmckerry bmckerry commented May 1, 2025

To horizontally scale the US transactions cluster, we're trying to give topicctl specific assignments for topics in the cluster via static placement (see docs).
Currently, running a topicctl apply --rebalance on a topic with static replica assignments is getting this error:

[2025-05-01 15:31:40]  INFO Here are the number of partitions per broker now, during the migration, and after:
---------+------------+------------+-------------
  BROKER |    CURR    |    MAX     |  PROPOSED
         | PARTITIONS | PARTITIONS | PARTITIONS
---------+------------+------------+-------------
  0      | 128        | 128        |  32 (-96)
  1      | 128        | 128        |  43 (-85)
  2      | 128        | 128        |  32 (-96)
  3      | 0          |  32 (+32)  |  32 (+32)
  4      | 0          |  42 (+42)  |  42 (+42)
  5      | 0          |  43 (+43)  |  43 (+43)
  6      | 0          |  32 (+32)  |  32 (+32)
  7      | 0          |  43 (+43)  |  43 (+43)
  8      | 0          |  42 (+42)  |  42 (+42)
  9      | 0          |  43 (+43)  |  43 (+43)
---------+------------+------------+-------------
[2025-05-01 15:31:40]  INFO They will be applied in batches of 1 partitions each, with a throttle of 512000000000000 bytes/sec (512000000 MB/sec)
[2025-05-01 15:31:40]  INFO Skipping update because dryRun is set to true
[2025-05-01 15:31:40]  INFO Checking leaders...
[2025-05-01 15:31:40]  INFO Running rebalance...
[2025-05-01 15:31:40] ERROR Error detected while creating or updating a topic
[2025-05-01 15:31:40] ERROR starting assignments on topic ingest-transactions-2 do not satisfy placement config

If I'm understanding the code correctly, the current implementation of Rebalance() using PlacementStrategyStatic makes it so that a rebalance will never happen, as it throws an error if the current replica assignments don't match the static assignments being specified.
I'm not sure what the purpose of this is, so this PR changes the case where current placement != desired static placement to just log something instead of erroring. More details below.

Comment on lines +72 to +75
// satify the placement config
if f.placementConfig.Strategy == config.PlacementStrategyStatic {
log.Info("Current partition assignment does not match static assignment, rebalancing will take place")
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

When applying a new topic, if EvaluateAssignments returns false, topicctl knows it needs to adjust the partition placement of the new topic to match the desired placement strategy: https://github.com/getsentry/topicctl/blob/master/pkg/apply/apply.go#L831

When rebalancing a topic (which is what this frequency.go file is used for), if EvaluateAssignments returns false, topicctl considers that to be an error. I've updated it so that if we're using static placement, topicctl doesn't error out here when rebalancing.

This reverts commit 4020cb9.
Copy link
Member

@evanh evanh left a comment

Choose a reason for hiding this comment

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

Looks good!

@bmckerry bmckerry merged commit d60ee36 into master May 1, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants