[DP-1762] - topicctl get actions for under replicated and offline partitions #149

ssingudasu · 2023-09-15T04:24:39Z

Description

This PR is to add new actions for topicctl get

Expectations:

topicctl get under-replicated-partitions (optional --topics flag)
topicctl get offline-partitions (optional --topics flag)
Fix the debug PersistentPreRunE: getPreRun in topicctl get since topicctl get is overriding the root cmd PersistentPreRunE: preRun

under replicated partitions

Kafka cluster is in under replicated state if the number of ISR are less than the Replicas available for the partition
topicctl already has readily available metadata call with filtered information. We will leverage topicctl admin pkg for under-replicated-partitions

offline partitions

topicctl has admin.GetTopics which performs kafka Metadata call, there is a lot of formatting and missing information in admin.GetTopics. Hence we will perform the kafka-go client.Metadata to ensure we capture all the offline partition information.
We leverage kafka-go metadata for offline-partitions

kafka-go metadata call for offline partitions gives the Leader partition information as below:

kafka "github.com/segmentio/kafka-go"

partition:

{
	Topic: topic_name
	ID: partition_id
	Leader: kafka.Broker (refer below)
	Replicas: [][Broker
	Isr:  [][Broker
	Error: kafka.Error
}

partition.Leader Broker response:

kafka.Broker{
        Host: ""
	Port: 0
	ID: 0
	Rack: ""
}

Unlike sarama which gives the offline leader broker id as -1, kafka-go gives the offline broker id as 0

Hence we will check the below error code: ListenerNotFound along with broker.Host as "" and broker.Port as 0 for partitions information in the metadata response

error message: "there is no listener on the leader broker that matches the listener on which metadata request was processed"
kafka.Error reference: https://github.com/segmentio/kafka-go/blob/main/error.go#L87

Expectation: if kafka.Error is ListenerNotFound and there is no broker or port found for leader partition, there by confirming that the partition is offline. We will display the ISR.ID and Repicas.ID with information from the kafka-go metadata call (i.e: each offline partition - Isr.ID and Replicas.ID as 0)

…titions

petedannemann

Good start!

petedannemann · 2023-09-15T13:25:21Z

cmd/topicctl/subcmd/get.go

 func init() {
+	log.SetFormatter(&prefixed.TextFormatter{


This log format and the debug flag should already be getting set by the root command that these subcommands are added to

topicctl/cmd/topicctl/subcmd/root.go

Line 31 in 7ac2269

RootCmd.PersistentFlags().BoolVar(

Ah I see, we are overriding them, nevermind

I think we can remove some duplicate code by adding this to the get subcommands prerun

rootCmd.PersistentPreRun(cmd, args)

spf13/cobra#252 (comment)

petedannemann · 2023-09-15T13:25:58Z

cmd/topicctl/subcmd/get.go

+	urpCommand := &cobra.Command{
+		Use:   "under-replicated-partitions",
+		Short: "Fetch all under replicated partitions in the kafka cluster.",
+		Args:  cobra.MinimumNArgs(0),


I think you want cobra.NoArgs, right?

petedannemann · 2023-09-15T13:26:03Z

cmd/topicctl/subcmd/get.go

+	opCommand := &cobra.Command{
+		Use:   "offline-partitions",
+		Short: "Fetch all offline partitions in the kafka cluster.",
+		Args:  cobra.MinimumNArgs(0),


I think you want cobra.NoArgs, right?

petedannemann · 2023-09-15T13:28:26Z

pkg/admin/types.go

+
+type TopicOPsInfo struct {
+	Name       string            `json:"name"`
+	Partitions []kafka.Partition `json:"partitions"`


I think we should be converting []kafka.Partition into []PartitionInfo for consistency

petedannemann · 2023-09-15T13:30:54Z

pkg/cli/cli.go

+	c.startSpinner()
+	urpsFound := false
+
+	filterTopics := make(map[string]bool)


In this file we usually put all the business logic into a method on the adminClient. This allows us to have different implementations for Kafka and Zookeeper. I think this would lead to some strange errors if someone tried to use this with Zookeeper right now. If we move this into a method on the adminClient we have ways to mark methods as unsupported and there will be consistent error messaging around that

So it would look something like this for Kafka. The Zookeeper implementation would return nil and we'd find some way to mark this as unsupported using our GetSupportedFeatures

petedannemann · 2023-09-15T15:14:19Z

Could we document these new commands in the README with the other get commands?

petedannemann · 2023-09-15T15:55:45Z

pkg/admin/types.go

@@ -136,6 +137,16 @@ type zkChangeNotification struct {
 	EntityPath string `json:"entity_path"`
 }

+type TopicURPsInfo struct {


The more I think about it, do we even need these new types? Couldn't we just reuse TopicInfo and leave the unused fields blank?

hhahn-tw · 2023-09-15T22:06:05Z

cmd/topicctl/subcmd/get.go

+type urpCmdConfig struct {
+	topics []string
+}
+
+var urpConfig urpCmdConfig
+
+type opCmdConfig struct {
+	topics []string
+}
+
+var opConfig urpCmdConfig


Using abbreviations for under-replicated-partitions and offline-partitions breaks the pattern used everywhere else. Even within these two new commands, it's not consistent: you have urpCmdConfg, underReplicatedPartitionsCmd, GetUnderReplicatedPartitions, urpCommand, FormatURPs, etc.

I realize they are long but I think it's better not to use abbreviations and be more consistent.

hhahn-tw · 2023-09-15T22:12:48Z

cmd/topicctl/subcmd/get.go

@@ -44,6 +62,12 @@ func init() {
 		false,
 		"Sort by value instead of name; only applies for lags at the moment",
 	)
+	getCmd.PersistentFlags().BoolVar(


--debug is noted as being a "global flag". Isn't this already set somewhere higher up the stack?

hhahn-tw · 2023-09-15T22:19:12Z

cmd/topicctl/subcmd/get.go

+func underReplicatedPartitionsCmd() *cobra.Command {
+	urpCommand := &cobra.Command{
+		Use:   "under-replicated-partitions",
+		Short: "Fetch all under replicated partitions in the kafka cluster.",


The term "under-replicated partitions", when output, is inconsistently written. I would suggest "under-replicated partitions" (with the hyphen between under and replicated, and not capitalized) because that is most grammatically correct IMO.

hhahn-tw · 2023-09-15T22:19:25Z

cmd/topicctl/subcmd/get.go

+		"topics",
+		"t",
+		[]string{},
+		"under replicated partitions for the topics",


under-replicated partitions

hhahn-tw · 2023-09-15T22:19:37Z

pkg/cli/cli.go

+	c.printer("Under Replicated Partitions:\n%s", admin.FormatURPs(allTopicURPs))
+
+	if urpsFound {
+		return fmt.Errorf("Topics have Under Replicated Partitions")


under-replicated partitions

hhahn-tw · 2023-09-15T22:24:55Z

pkg/cli/cli.go

+
+	c.printer("Under Replicated Partitions:\n%s", admin.FormatURPs(allTopicURPs))
+
+	if urpsFound {


Is urpsFound necessary? Can't you use len(allTopicURPs)>0?

hhahn-tw · 2023-09-15T22:25:38Z

pkg/cli/cli.go

+
+	c.printer("Offline Partitions:\n%s", admin.FormatOPs(allTopicOPs))
+
+	if opsFound {


as above, is opsFound necessary versus len(allTopicOPs)>0?

hhahn-tw · 2023-09-15T22:58:48Z

pkg/cli/cli.go

+	// If ListenerNotFound Error occurs for leader partition
+	// this means the partition is offline
+	var listenerNotFoundError kafka.Error
+	listenerNotFoundError = 72


It's strange to see a hard coded value here. Can't it be referenced by name?

ssingudasu · 2023-09-20T16:07:11Z

Closing this PR as we have decided to implement the same in

# topicctl get partitions

[DP-1762] - topicctl get actions for under replicated and offline par…

e3452c4

…titions

ssingudasu requested a review from a team as a code owner September 15, 2023 04:24

ssingudasu added 2 commits September 14, 2023 21:25

[DP-1762] - Minor bug fixes

6aa2e0f

[DP-1762] - Minor bug fixes

1054bf9

petedannemann reviewed Sep 15, 2023

View reviewed changes

hhahn-tw reviewed Sep 15, 2023

View reviewed changes

ssingudasu closed this Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DP-1762] - topicctl get actions for under replicated and offline partitions #149

[DP-1762] - topicctl get actions for under replicated and offline partitions #149

ssingudasu commented Sep 15, 2023 •

edited

Loading

petedannemann left a comment

petedannemann Sep 15, 2023

petedannemann Sep 15, 2023

petedannemann Sep 15, 2023

petedannemann Sep 15, 2023

petedannemann Sep 15, 2023

petedannemann Sep 15, 2023

petedannemann Sep 15, 2023

petedannemann Sep 15, 2023

petedannemann commented Sep 15, 2023

petedannemann Sep 15, 2023

hhahn-tw Sep 15, 2023

hhahn-tw Sep 15, 2023

hhahn-tw Sep 15, 2023

hhahn-tw Sep 15, 2023

hhahn-tw Sep 15, 2023

hhahn-tw Sep 15, 2023

hhahn-tw Sep 15, 2023

hhahn-tw Sep 15, 2023

ssingudasu commented Sep 20, 2023


		c.printer("Under Replicated Partitions:\n%s", admin.FormatURPs(allTopicURPs))

		if urpsFound {


		c.printer("Offline Partitions:\n%s", admin.FormatOPs(allTopicOPs))

		if opsFound {

[DP-1762] - topicctl get actions for under replicated and offline partitions #149

[DP-1762] - topicctl get actions for under replicated and offline partitions #149

Conversation

ssingudasu commented Sep 15, 2023 • edited Loading

Description

Expectations:

under replicated partitions

offline partitions

petedannemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petedannemann commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssingudasu commented Sep 20, 2023

ssingudasu commented Sep 15, 2023 •

edited

Loading