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

topicctl new action: rebalance #142

Merged
merged 21 commits into from
Jul 11, 2023

Conversation

ssingudasu
Copy link
Contributor

@ssingudasu ssingudasu commented Jun 7, 2023

Introduction:

To rebalance a topic in topicctl, We currently use topicctl apply $topic --rebalance / topicctl apply $topic --rebalance --to-remove . In a production scale Kafka cluster, there is a need to rebalance all topics for a cluster.

Goal:

  • Add new action - rebalance to topicctl. i.e topicctl rebalance
  • Print rebalance progress status (stdout) during rebalance. Refer log [timestamp] INFO Rebalance Progress: {}. (Please refer below screenshots for output)
  • Rebalance progress information is two fold. overall topic rebalance status and topic rebalance status for each iteration

Considerations for rebalance:

  • expects --cluster-config (i.e cluster.yaml) and --path-prefix (topics folder)
  • Uses underlying topicctl pkg/apply to rebalance a topic
  • Argument --show-progress-interval to show rebalance progress in intervals
  • Arguments --skip-confirm --auto-continue-rebalance are set to true by default
  • Argument --retention-drop-step-duration is set to default value. Hence that argument is not considered for rebalance

NOTE: A topic is not considered for rebalance if

  1. topic config is inconsistent with cluster config (name, region, environment etc...)
  2. partitions of a topic in kafka cluster does not match with topic partition setting in topic config
  3. retention.ms of a topic in kafka cluster does not match with topic retentionMinutes setting in topic config
  4. topic does not exist in kafka cluster

Test Output:

Tested on local kafka cluster (3 brokers, 1 in each rack) with 3 topics
test-1 -> exists in kafka. But settings mismatch with test-1 topic yaml config
test-2 -> exists in kafka. settings match (replication 2)
test-3 -> does not exist in kafka
test-4 -> exists in kafka. settings match (replication 1)


go run cmd/topicctl/main.go -h

Screenshot 2023-06-06 at 19 38 25

go run cmd/topicctl/main.go rebalance -h

Screenshot 2023-06-06 at 19 39 10

go run cmd/topicctl/main.go rebalance

Screenshot 2023-06-06 at 19 40 08

go run cmd/topicctl/main.go rebalance --cluster-config /path/to/cluster/cluster.yaml --path-prefix /path/to/cluster/topics/ --show-progress-interval 5s --sleep-loop-duration 15s --partition-batch-size 5 --broker-throttle-mb 640

Screenshot 2023-06-06 at 19 46 55

go run cmd/topicctl/main.go rebalance --cluster-config /path/to/cluster/cluster.yaml --path-prefix /path/to/cluster/topics/ --show-progress-interval 5s --sleep-loop-duration 15s --partition-batch-size 5 --broker-throttle-mb 640 --to-remove 3

Screenshot 2023-06-06 at 19 50 46 Screenshot 2023-06-06 at 19 50 59 Screenshot 2023-06-06 at 19 51 14 Screenshot 2023-06-06 at 19 51 27 Screenshot 2023-06-06 at 19 51 39 Screenshot 2023-06-06 at 19 51 53 Screenshot 2023-06-06 at 19 52 08 Screenshot 2023-06-06 at 19 52 38 Screenshot 2023-06-06 at 19 53 13

go run cmd/topicctl/main.go rebalance --cluster-config /path/to/cluster/cluster.yaml --path-prefix /path/to/cluster/topics/ --show-progress-interval 5s --sleep-loop-duration 15s --partition-batch-size 5 --broker-throttle-mb 640 --to-remove 2,3

Screenshot 2023-06-06 at 19 56 50 Screenshot 2023-06-06 at 19 57 16 Screenshot 2023-06-06 at 19 57 31 Screenshot 2023-06-06 at 19 57 46

@ssingudasu ssingudasu requested a review from a team as a code owner June 7, 2023 02:32
pkg/apply/apply.go Outdated Show resolved Hide resolved
pkg/util/progress.go Outdated Show resolved Hide resolved
cmd/topicctl/subcmd/rebalance.go Outdated Show resolved Hide resolved
cmd/topicctl/subcmd/rebalance.go Outdated Show resolved Hide resolved
cmd/topicctl/subcmd/rebalance.go Outdated Show resolved Hide resolved
pkg/util/progress.go Outdated Show resolved Hide resolved
cmd/topicctl/subcmd/rebalance.go Outdated Show resolved Hide resolved
cmd/topicctl/subcmd/rebalance.go Outdated Show resolved Hide resolved
pkg/util/progress.go Outdated Show resolved Hide resolved
pkg/util/progress.go Outdated Show resolved Hide resolved
cmd/topicctl/subcmd/rebalance.go Outdated Show resolved Hide resolved
cmd/topicctl/subcmd/rebalance.go Show resolved Hide resolved
pkg/util/progress.go Outdated Show resolved Hide resolved
cmd/topicctl/subcmd/rebalance.go Outdated Show resolved Hide resolved
cmd/topicctl/subcmd/rebalance.go Outdated Show resolved Hide resolved
pkg/apply/apply.go Outdated Show resolved Hide resolved
pkg/util/progress.go Outdated Show resolved Hide resolved
Copy link
Contributor

@hhahn-tw hhahn-tw left a comment

Choose a reason for hiding this comment

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

I think something should also be added to the rebalance section of the readme to emphasize that rebalance is a new verb in addition to apply for this functionality, and briefly explain how it gathers a list of topics based on yaml files found in the specified location.

README.md Outdated
topicctl rebalance [flags]
```

The `rebalance` subcommand will do a full broker rebalance for all the topic configs in a given topic prefix path.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming you just used the wording from the rebalance section but "full broker rebalance" suggests something else to me. How about "full cluster rebalance" or "all broker rebalance"? This change should be made in all places it appears.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"full cluster rebalance" this makes sense to me..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about modifying at other places. But I am modifying atleast in the subcommand rebalance

@ssingudasu ssingudasu requested a review from hhahn-tw July 11, 2023 18:56
@ssingudasu ssingudasu merged commit d1378c0 into master Jul 11, 2023
14 checks passed
@ssingudasu ssingudasu deleted the ssingudasu/dp-1413_topiccl_new_action_rebalance branch July 11, 2023 20:50
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.

None yet

4 participants