Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13649 )
Change subject: [tools] Add get/set extra-configs for CLI tools ...................................................................... Patch Set 2: (5 comments) Overall looks good! A few nits and some testing suggestions. http://gerrit.cloudera.org:8080/#/c/13649/2/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13649/2/src/kudu/tools/kudu-admin-test.cc@2259 PS2, Line 2259: ASSERT_TOOL_OK( : "table", : "set_extra_config", : master_address, : kTableId, : "kudu.table.history_max_age_sec", : "3600" : ); : : string stdout, stderr; : Status s = RunKuduTool({ : "table", : "get_extra_configs", : master_address, : kTableId : }, &stdout, &stderr); : ASSERT_TRUE(s.ok()) << ToolRunInfo(s, stdout, stderr); : ASSERT_STR_CONTAINS(stdout, "kudu.table.history_max_age_sec | 3600"); Could you also test the behavior when: - no extra config is set - when requesting a config that doesn't exist, e.g. "foobar" http://gerrit.cloudera.org:8080/#/c/13649/2/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13649/2/src/kudu/tools/tool_action_table.cc@86 PS2, Line 86: extra_config_tags, "", : "Comma-separated list of tags used to restrict which extra-configs are returned. " : "An empty value matches all tags" nit: "tag" is a little vague since we don't use it anywhere currently. How about calling this 'config_names'? and something along the lines of: "Comma-separated list of configurations to display. An empty value displays all configs." http://gerrit.cloudera.org:8080/#/c/13649/2/src/kudu/tools/tool_action_table.cc@449 PS2, Line 449: key nit: How about "Configuration" and "Value"? http://gerrit.cloudera.org:8080/#/c/13649/2/src/kudu/tools/tool_action_table.cc@566 PS2, Line 566: Value nit: lower case http://gerrit.cloudera.org:8080/#/c/13649/2/src/kudu/tools/tool_action_table.cc@573 PS2, Line 573: Name of the table to alter nit: We're not altering the table here. Perhaps: "Name of the table for which to get extra configurations." -- To view, visit http://gerrit.cloudera.org:8080/13649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I90c790bbfe41a59f621157ff6b3f11d2b8f916e7 Gerrit-Change-Number: 13649 Gerrit-PatchSet: 2 Gerrit-Owner: Yao Xu <oclarms....@gmail.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 19 Jun 2019 03:35:51 +0000 Gerrit-HasComments: Yes
