Skip to content

fix issues from get subcmd refactor #151

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

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

petedannemann
Copy link
Contributor

@petedannemann petedannemann commented Sep 18, 2023

#148 introduced two new (known) bugs that are fixed in this PR:

  1. The debug command broke for the get subcommand. This is due to PersistentPreRun only traversing one level of subcommands. This is a known issue with cobra
  2. Zookeeper get commands broke. This commit closes the adminclient before the function returns with the defer command, which is not what we want to do. We must keep the adminclient open until the end of the command so this PR reverts that commit which added the buggy getCliRunnerAndCtx function

Verification:

  1. Debug flag fix

Before:

go run cmd/topicctl/main.go get topics --cluster-config examples/local-cluster/cluster.yaml --debug
[2023-09-18 08:55:33]  INFO Topics:
-------+------------+-------------+-----------+------------
  NAME | PARTITIONS | REPLICATION | RETENTION |   RACKS
       |            |             |   MINS    | (MIN,MAX)
-------+------------+-------------+-----------+------------
-------+------------+-------------+-----------+------------

After:

$ go run cmd/topicctl/main.go get topics --cluster-config examples/local-cluster/cluster.yaml --debug
[2023-09-18 08:55:29] DEBUG No ZK addresses provided, using broker admin client
[2023-09-18 08:55:29] DEBUG Connecting to cluster on address localhost:9092 with TLS enabled=false, SASL enabled=false
[2023-09-18 08:55:29] DEBUG Getting supported API versions
...
  1. Zk Fix

Before:

$ go run cmd/topicctl/main.go get brokers --zk-addr localhost:2181
panic: send on closed channel

goroutine 1 [running]:
github.com/segmentio/topicctl/pkg/zk.(*PooledClient).Children(0xc00017c570, {0x1ba3770, 0xc000122000}, {0xc00062e0c0, 0xc})
        /Users/pdannemann/src/topicctl/pkg/zk/client.go:182 +0x12e
github.com/segmentio/topicctl/pkg/admin.(*ZKAdminClient).GetBrokerIDs(0xc000212280, {0x1ba3770, 0xc000122000})
        /Users/pdannemann/src/topicctl/pkg/admin/zkclient.go:274 +0xb6
github.com/segmentio/topicctl/pkg/admin.(*ZKAdminClient).GetBrokers(0xc000212280, {0x1ba3770, 0xc000122000}, {0x0?, 0x0?, 0xc0004dc480?})
        /Users/pdannemann/src/topicctl/pkg/admin/zkclient.go:174 +0x53
github.com/segmentio/topicctl/pkg/cli.(*CLIRunner).GetBrokers(0xc0004dc480, {0x1ba3770?, 0xc000122000?}, 0x0?)
        /Users/pdannemann/src/topicctl/pkg/cli/cli.go:69 +0x85
github.com/segmentio/topicctl/cmd/topicctl/subcmd.brokersCmd.func1(0xc0004fe000?, {0xc0004dc200?, 0x2?, 0x2?})
        /Users/pdannemann/src/topicctl/cmd/topicctl/subcmd/get.go:121 +0x49
github.com/spf13/cobra.(*Command).execute(0xc0004fe000, {0xc0004dc1e0, 0x2, 0x2})
        /Users/pdannemann/dev/pkg/mod/github.com/spf13/[email protected]/command.go:872 +0x694
github.com/spf13/cobra.(*Command).ExecuteC(0x1feae60)
        /Users/pdannemann/dev/pkg/mod/github.com/spf13/[email protected]/command.go:990 +0x3b4
github.com/spf13/cobra.(*Command).Execute(...)
        /Users/pdannemann/dev/pkg/mod/github.com/spf13/[email protected]/command.go:918
github.com/segmentio/topicctl/cmd/topicctl/subcmd.Execute({0x1838289?, 0xc0000021a0?})
        /Users/pdannemann/src/topicctl/cmd/topicctl/subcmd/root.go:49 +0xb1
main.main()
        /Users/pdannemann/src/topicctl/cmd/topicctl/main.go:13 +0x27

After:

$ go run cmd/topicctl/main.go get brokers --zk-addr localhost:2181
[2023-09-18 08:56:22]  INFO Brokers:
-----+-----------+------+------+-----------------------
  ID |   HOST    | PORT | RACK |      TIMESTAMP
-----+-----------+------+------+-----------------------
  1  | localhost | 9092 |      | 2023-09-16T17:06:33Z
-----+-----------+------+------+-----------------------
[2023-09-18 08:56:22]  INFO Brokers per rack:
-------+--------------
  RACK | NUM BROKERS
-------+--------------
       | 1
-------+--------------

@petedannemann petedannemann marked this pull request as ready for review September 18, 2023 13:04
@petedannemann petedannemann requested a review from a team as a code owner September 18, 2023 13:04
@petedannemann petedannemann changed the title fix/issues from get subcmd refactor fix issues from get subcmd refactor Sep 18, 2023
Copy link
Contributor

@ssingudasu ssingudasu left a comment

Choose a reason for hiding this comment

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

LGTM

@petedannemann petedannemann merged commit a16112f into master Sep 18, 2023
@petedannemann petedannemann deleted the fix/issues-from-get-subcmd-refactor branch September 18, 2023 17:25
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.

2 participants