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

feat: add consumer group describe command #536

Merged
merged 9 commits into from
Apr 15, 2021

Conversation

rkpattnaik780
Copy link
Contributor

@rkpattnaik780 rkpattnaik780 commented Apr 8, 2021

Description

Subcommand to view a consumer group.

rhoas kafka consumergroup describe --id consumer_group_1

Screenshot from 2021-04-08 15-24-07

fixes #385

Verification Steps

  1. Run make mock-api/start to start the mock API.
  2. Run go run ./cmd/rhoas login --api-gateway=http://localhost:8000 to use the mock API.
  3. Run go run ./cmd/rhoas kafka use somekafka to use a Kafka instance (this will always pick the same one, it is not dynamic).
  4. Run go run ./cmd/rhoas kafka consumergroup describe consumer_group_1 to view a consumer group.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • Documentation added for the feature
  • CI and all relevant tests are passing
  • Code Review completed
  • Verified independently by reviewer

Copy link
Contributor

@craicoverflow craicoverflow left a comment

Choose a reason for hiding this comment

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

❯ ./rhoas kafka consumergroup describe --id consumer_group_1
Consumer group ID: consumer_group_1
  MEMBER ID                PARTITION   LOG END OFFSET   CURRENT OFFSET   OFFSET LAG  
 ------------------------ ----------- ---------------- ---------------- ------------ 
  consumer_group_member1           0                5                5            0  
  consumer_group_member2           1                3                3            0

I'd like to see this formatted a little bit better - a space between the top level info and the table headers will make it easier to read.

The UI also has Active members and Partitions with Lag at the top level, should we do the same?

},
}

cmd.Flags().StringVar(&opts.id, "id", "", localizer.MustLocalizeFromID("kafka.consumerGroup.describe.flag.id.description"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should use a positional argument instead: rhoas kafka consumergroup describe consumer_group_1. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It occured to me, dropped the idea since we use positional arg for names and flags for id. I would suggest we go with --id flag or both flag and argumnet, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

We use --id for service accounts only, but this is because service accounts have a name also, and service account ID is a UUID which is not friendly to dynamic auto completion, whereas consumer group IDs are, so I think we should use positionals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, sounds fair.
Let us stick to positionals.

pkg/cmd/kafka/consumergroup/describe/describe.go Outdated Show resolved Hide resolved
@rkpattnaik780
Copy link
Contributor Author

❯ ./rhoas kafka consumergroup describe --id consumer_group_1
Consumer group ID: consumer_group_1
  MEMBER ID                PARTITION   LOG END OFFSET   CURRENT OFFSET   OFFSET LAG  
 ------------------------ ----------- ---------------- ---------------- ------------ 
  consumer_group_member1           0                5                5            0  
  consumer_group_member2           1                3                3            0

I'd like to see this formatted a little bit better - a space between the top level info and the table headers will make it easier to read.

The UI also has Active members and Partitions with Lag at the top level, should we do the same?

Yes that would be nice, missed it somehow. We should keep it as similar to UI as possible

Copy link
Contributor

@craicoverflow craicoverflow left a comment

Choose a reason for hiding this comment

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

Code looks great, but some changes required in how we display stuff.

[kafka.consumerGroup.describe.cmd.example]
one = '''
# describe a consumer group
$ rhoas kafka consumergroup describe --id consumer_group_1 -o json
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs updating.

Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) (err error) {

opts.id = args[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

We should having dynamic auto completion for the ID.

Comment on lines 149 to 166
fmt.Fprintln(stdout, localizer.MustLocalize(&localizer.Config{
MessageID: "kafka.consumerGroup.describe.output.id",
TemplateData: map[string]interface{}{
"ID": consumerGroupData.GetId(),
},
}))
fmt.Fprintln(stdout, localizer.MustLocalize(&localizer.Config{
MessageID: "kafka.consumerGroup.describe.output.activeMembers",
TemplateData: map[string]interface{}{
"ActiveMembers": len(consumerGroupData.GetConsumers()),
},
}))
fmt.Fprintln(stdout, localizer.MustLocalize(&localizer.Config{
MessageID: "kafka.consumerGroup.describe.output.partitionsWithLag",
TemplateData: map[string]interface{}{
"LaggingPartitions": consumergroup.GetPartitionsWithLag(consumerGroupData.GetConsumers()),
},
}))
Copy link
Contributor

Choose a reason for hiding this comment

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

We should explore a neater approach for this - there is no alignment in the indentation and it is a bit difficult to read in my opinion. Check out the formatting of rhoas status to see whether the same could be used here.

@craicoverflow craicoverflow force-pushed the consumer_group_describe branch from f4c503e to 635665f Compare April 15, 2021 10:03
@craicoverflow craicoverflow force-pushed the consumer_group_describe branch from 635665f to 1e237e8 Compare April 15, 2021 16:14
@craicoverflow craicoverflow force-pushed the consumer_group_describe branch from 9ce7da4 to f835a5d Compare April 15, 2021 16:26
@craicoverflow craicoverflow changed the title Consumer group describe feat: add consumer group describe command Apr 15, 2021
@craicoverflow craicoverflow merged commit c0beed7 into main Apr 15, 2021
@craicoverflow craicoverflow deleted the consumer_group_describe branch April 15, 2021 16:37
craicoverflow pushed a commit that referenced this pull request Apr 20, 2021
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.

Add the ability to view a consumer group
2 participants