twmb / kcl

Your one stop shop to do anything with Kafka. Producing, consuming, transacting, administrating; 0.8.0 through 3.2+
BSD 3-Clause "New" or "Revised" License
194 stars 19 forks source link

feature: list topics command #7

Closed SteadBytes closed 3 years ago

SteadBytes commented 3 years ago

It's often useful to be able to list all available topics. kcl could support this with a kcl topic list command:

kcl topic list
Topic   Partitions  Internal?
topic1  3           false
topic2  3           false
topic3  3           false

It could also include optional internal topic filtering. What do you think?

There's some potential complication with handling Kafka versions < 0.10.0 due to kmsg.MetadataRequest.Topics taking nil or and empty slice to indicate all topics but the gist of the command is something like:

func topicListCommand(cl *client.Client) *cobra.Command {
    cmd := &cobra.Command{
        Use:   "list",
        Short: "List all topics (Kafka 0.10.0.0+)",
        Run: func(_ *cobra.Command, topics []string) {
            req := kmsg.NewMetadataRequest()
            kresp, err := cl.Client().Request(context.Background(), &req)
            out.MaybeDie(err, "unable to list topics: %v", err)
            resp := kresp.(*kmsg.MetadataResponse)
            if cl.AsJSON() {
                out.ExitJSON(resp.Topics)
            }
            tw := out.BeginTabWrite()
            defer tw.Flush()
            fmt.Fprintln(tw, "Topic\tPartitions\tInternal?")
            for _, topicResp := range resp.Topics {
                fmt.Fprintf(tw, "%s\t%d\t%t\n", topicResp.Topic, len(topicResp.Partitions), topicResp.IsInternal)
            }
        },
    }
    return cmd
}

Happy to submit a PR for implementation if desired πŸ˜„

twmb commented 3 years ago

kcl actually has this ability! But it's not as obvious as kcl topic list. The current command to do this is kcl metadata -t, and you can specify individual topics as additional arguments (vs. the default of all topics).

I could potentially add an alias here / add topic list directly. The reason I went with metadata is that (as you saw) the only way to list topics is through a metadata request, which also returns a bunch of other useful information -- so kcl metadata has flags to opt in to print which information you want (I usually do kcl metadata -tib).

What do you think of this? It may still be worth it to have kcl topic list, though.

SteadBytes commented 3 years ago

Woops! Apologies for missing that - I see now from kcl metadata -h :facepalm:

I don't disagree with your reasoning around using kcl metadata for this, however could providing kcl topic list as essentially an alias for kcl metadata -t be useful? It seems more intuitive, at least in my opinion, to have create, list and delete actions together.

That said, the main reason for the issue was to have the functionality at all. Perhaps, if nothing else, including some examples of kcl metadata in the README example usages would help expose the functionality to users?

twmb commented 3 years ago

I think kcl topic list may be a fine addition! Would you like to create a PR, or I could do it?

twmb commented 3 years ago

Also README examples may be good too!

SteadBytes commented 3 years ago

I'm happy to put up a PR πŸ˜„ Do you think kcl topic list should literally be an alias for kcl metadata -t? e.g. directly run metadata.Command with "-t" in the args list - something like:

func topicListCommand(cl *client.Client) *cobra.Command {
    cmd := &cobra.Command{
        Use:     "list",
        Aliases: []string{"l"},
        Short:   "List all topics",
        Long:    "List all topics (alias for metadata -t)",
        Run: func(_ *cobra.Command, _ []string) {
            cmd := metadata.Command(cl)
            cmd.SetArgs([]string{"-t"})
            cmd.Execute()
        },
    }
    return cmd
}

Or is it worth pulling that part of the metadata command out to be shared?

twmb commented 3 years ago

I didn't think it'd be that easy, that looks like a great choice. I'm open for whatever is easiest.

It's probably also worth it to include support for the -d flag for detailed output, but I think that could just be if detailed { cmd.SetArgs... }.

What do you think? Otherwise I'm onboard with that.

For aliases I usually add ls for list ones, but l can work too -- if you want to add both?

SteadBytes commented 3 years ago

Great!

It's probably also worth it to include support for the -d flag for detailed output

Yes the -d flag is definitely a useful addition.

For aliases I usually add ls for list ones, but l can work too -- if you want to add both?

If ls is the common pattern then it's probably best to stick with that πŸ‘

I'll get a PR up tomorrow morning ☺️