-
Notifications
You must be signed in to change notification settings - Fork 70
Add topic operation commands #56
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
Conversation
sijie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request contains too many unrelated groups of commands. I would suggest breaking it down into multiple pull requests.
- offload
- terminate
- compaction
- status
| "so please make sure the service urls provided in this command are reachable\n" + | ||
| "between clusters.\n" + | ||
| "\n" + | ||
| "This command is used for adding the configuration data for a cluster.\n"+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you are change formats in different pull requests. sometimes you add spaces and sometimes delete the spaces. I would suggest avoiding including style or code-convention changes in a huge pull request. if we need enforce a certain code convention, please enforce in the CI. /cc @wolfstudy
| func CheckTopicNameTwoArgs(args []string) error { | ||
| if len(args) != 2 { | ||
| return errors.New("need to specified the topic name and the partitions") | ||
| return errors.New("only two argument is allowed to be used as names") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you change the message here? If this method is used for other purpose, please create a new method and make sure the error message is self-explained and specific.
| admin := cmdutils.NewPulsarClient() | ||
| err = admin.Topics().Compact(*topic) | ||
| if err == nil { | ||
| vc.Command.Printf("Sending compact topic %s request successfully/n", topic.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| vc.Command.Printf("Sending compact topic %s request successfully/n", topic.String()) | |
| vc.Command.Printf("Started compacting topic %s successfully/n", topic.String()) |
|
|
||
| func CompactStatusCmd(vc *cmdutils.VerbCmd) { | ||
| var desc LongDescription | ||
| desc.CommandUsedFor = "This command is used for getting status of compaction on a topic." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| desc.CommandUsedFor = "This command is used for getting status of compaction on a topic." | |
| desc.CommandUsedFor = "This command is used for getting the compaction status of a topic." |
Please fix the example output as well.
|
|
||
| var examples []Example | ||
| offload := Example{ | ||
| Desc: "Trigger offload the data from a topic <topic-name> to long-term storage and " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Desc: "Trigger offload the data from a topic <topic-name> to long-term storage and " + | |
| Desc: "Trigger offloading the data from a topic <topic-name> to a long-term storage and " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also fix all the output examples.
| var examples []Example | ||
| offload := Example{ | ||
| Desc: "Trigger offload the data from a topic <topic-name> to long-term storage and " + | ||
| "keep <threshold> size data in BookKeeper (e.g. 10M, 5G, default is byte)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep the configured amount of data in BookKeeper only
| return nil | ||
| } | ||
|
|
||
| func validateSize(s string) (int64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please find any existing libraries already did that?
| } | ||
|
|
||
| errorOut := Output{ | ||
| Desc: "Offload is error", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix this sentence
|
|
||
| notRun := Output{ | ||
| Desc: "Offloading is not running", | ||
| Out: "Offload has not been run for <topic-name> since broker startup", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the syntax
| } | ||
| } | ||
|
|
||
| switch status.Status { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure all the output sentences include namespace name.
compact
compact-status
offload
offload-status
unload
terminate