pd-ctl: add config replicate and show all config #573
Conversation
pdctl/command/config_command.go
Outdated
| @@ -115,3 +149,50 @@ func setConfigCommandFunc(cmd *cobra.Command, args []string) { | |||
| printResponseError(r) | |||
| } | |||
| } | |||
|
|
|||
| func setReplicateConfigCommandFunc(cmd *cobra.Command, args []string) { | |||
There was a problem hiding this comment.
can we use setConfigCommandFunc directly? I think the implementation code is duplicated here.
pdctl/command/config_command.go
Outdated
| value, err = strconv.ParseFloat(args[1], 64) | ||
| if err != nil { | ||
| value = args[1] | ||
| switch k := f.typ; true { |
There was a problem hiding this comment.
what's the purpose of this? I
There was a problem hiding this comment.
to convert the value to correct type, like LocationLabels are []string. the marshal need distinguish these.
There was a problem hiding this comment.
I prefer switch f.type here and list all numeric types instead of k <= reflect.Float64.
pdctl/command/config_command.go
Outdated
| r, err = http.Post(url, "application/json", bytes.NewBuffer(req)) | ||
| if path == schedulePrefix { | ||
| path = configPrefix | ||
| } |
There was a problem hiding this comment.
Why not use schedulePrefix?
There was a problem hiding this comment.
the API setSchedule and getSchedule are not consistent. considering compatible I just adjust it in here.
There was a problem hiding this comment.
I think it's ok to give up supporting old pd-server.
|
LGTM. |
pdctl/command/config_command.go
Outdated
| url := getAddressFromCmd(cmd, schedulePrefix) | ||
| var value interface{} | ||
| data := make(map[string]interface{}) | ||
| f, ok := optionRel[opt] |
There was a problem hiding this comment.
We should parse args for different sub commands separately, it is more clear than using reflect here.
IMO, I don't like reflect here.
There was a problem hiding this comment.
like config set schedule <option> <value> and config set replicas <option> <value>? but we also need to distinguish the field type like string and []string in ReplicationConfig. I think this way is ok, it is no need to adjust when I add a field in config.
pdctl/command/config_command.go
Outdated
|
|
||
| url := getAddressFromCmd(cmd, schedulePrefix) | ||
| func postDataWithPath(cmd *cobra.Command, args []string, path string) error { |
There was a problem hiding this comment.
If args len must be 2, use key string, value string instead
There was a problem hiding this comment.
/postDataWithPath/postConfigDataWithPath/s
| h.rd.JSON(w, http.StatusInternalServerError, err.Error()) | ||
| return | ||
| } | ||
| err = json.Unmarshal(data, &config.Schedule) |
There was a problem hiding this comment.
can we support json.Unmarshal(data, &config) later?
IMO, I prefer check key-value one by one.
There was a problem hiding this comment.
I think this is a more easy way :). the other config no need to dynamic adjust.
PTAL @siddontang @overvenus @disksing