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 3: (1 comment) Thanks for making the change! One more thing I missed last time. http://gerrit.cloudera.org:8080/#/c/13649/3/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13649/3/src/kudu/tools/tool_action_table.cc@455 PS3, Line 455: for (const auto& config_name : config_names) { : const string* config_value = FindOrNull(table->extra_configs(), config_name); : if (config_value) { : data_table.AddRow({ config_name, *config_value }); : } : } Should we instead iterate over the extra configs and to an unordered_set lookup? I think as written, if --config_names contains duplicates, we'll print duplicates. Might be worth a test too. -- 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: 3 Gerrit-Owner: Yao Xu <oclarms....@gmail.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yao Xu <oclarms....@gmail.com> Gerrit-Comment-Date: Wed, 19 Jun 2019 17:06:31 +0000 Gerrit-HasComments: Yes
