Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags ...................................................................... Patch Set 11: (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: NO_FATALS(StartExternalMiniCluster(std::move(opts))); > Maybe parameterize this instead so we don't lose non-kerberized coverage? M Done. On my machine none of the shards take more than 60s so I didn't increase it from 4. 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? This option follows directly from the protobuf interface[1] which this method directly calls, so I'd prefer not to change it. It's also used as a bool through the all_flags flag[2]. [1]: https://github.com/apache/kudu/blob/master/src/kudu/server/server_base.proto#L45 [2]: https://github.com/apache/kudu/blob/master/src/kudu/tools/tool_action_common.cc#L101 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 It's now been added to the optional arg list. http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@123 PS10, Line 123: > Is this even possible? Done 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(); : // SetCommandLineOption avoids needing to parse the value into a bool. : CHECK(!gflags::SetCommandLineOption("hive_metastore_sasl_enabled", > Why are these set via different methods? I can take my answer in the form o Done http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@143 PS10, Line 143: // Create HMS catalog. > As a TODO item, it would be nice if HmsCatalog (and maybe HmsClient) behave I agree in principal, but I think it's pretty nice that the low level timeout and buffer size flags are not exposed in the tool help output, but can still be overridden in exceptional circumstances. -- 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: 11 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 23:26:47 +0000 Gerrit-HasComments: Yes
