Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags ...................................................................... Patch Set 10: (6 comments) http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/kudu-tool-test.cc@2215 PS10, Line 2215: // Kerberos is enabled in order to test the tools work in secure clusters. Maybe parameterize this instead so we don't lose non-kerberized coverage? May need to increase the number of shards too. http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_common.h@114 PS10, Line 114: // 'all_flags' controls whether all flags are returned, or only flags which are : // explicitly set. Can you define a new enum for this instead? http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@114 PS10, Line 114: if (FLAGS_hive_metastore_uris.empty()) { See below: since --hive-metastore-uris isn't advertised anymore, we should assume that this is always true. http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@123 PS10, Line 123: hms_uris->value().empty() Is this even possible? http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@137 PS10, Line 137: FLAGS_hive_metastore_uris = hms_uris->value(); : CHECK(!gflags::SetCommandLineOption("hive_metastore_sasl_enabled", : hms_sasl_enabled->value().c_str()).empty()); Why are these set via different methods? I can take my answer in the form of a code comment. http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@143 PS10, Line 143: hms_catalog->reset(new hms::HmsCatalog(master_addrs_flag)); As a TODO item, it would be nice if HmsCatalog (and maybe HmsClient) behaved more like libraries and received configurable options via constructor rather than via gflags. Without that, I think it's still possible for a CLI user to set --hive-metastore-uris on the command line and for that to take effect, which doesn't make sense because that gflag is no longer advertised via AddOptionalParameter. -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 10 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 31 Jul 2018 17:38:42 +0000 Gerrit-HasComments: Yes
