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 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2193 PS1, Line 2193: master_addr > nit: master_addrs. And also in all other places. Pretty much all the test cases in this file are using 'master_addr', I think because the tests aren't configured to with multi-master. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2211 PS1, Line 2211: Kerberos is enabled > Do we need to parameterized this with Kerberos disabled/enabled? I opted to have this one be kerberized and TestCheckAndManualFixHmsMetadata not be kerberized. I think that will give us enough coverage without having to execute the test-cases twice. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_common.h@115 PS1, Line 115: // explicitly set. > Can you add a bit explanation/comment on what is the parameter for? And how Done http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@a599 PS1, Line 599: > The comment for the changelist says something like '... when it is not prov Yes, by removing them as optional parameters they will not be shown in the output of 'kudu hms check --help', however they are still settable. In effect, they are 'hidden'. I'd prefer them to not be shown, because the vast majority of the time they should be looked up on the master. The fallback is in place in case a different HMS address must be used, but I can't imagine at this point why you'd want to do that. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@113 PS1, Line 113: FLAGS_hive_metastore_uris.empty() > When the URIs and SASL flags will not be empty? Do we need to validate if t hive_metastore_uris won't be empty if it's set on the command line via a flag. In that case it's used instead of the master config because the user is setting it specifically. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116 PS1, Line 116: master_addrs[0] > Is it possible that different masters have different configs? Eventually I think we should add a check that all masters have the same configuration value, but I think that would better be handled as it's own check as a part of ksck or 'hms check'. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116 PS1, Line 116: master::Master::kDefaultPort > What happens if the port being used is different from the default? The provided port is the fallback in case the master address doesn't include a port. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@120 PS1, Line 120: hive_metastore_uris > nit: does it make sense to introduce constant literals for 'hive_metastore These are derivative from the associated flag definitions. Ideally there would be a way to get the flag name from the flag symbol (FLAGS_hive_metastore_uris -> 'hive_metastore_uris'), so that if the flag name ever changes this code would stop compiling. I can't figure out a way to do that, though, and I don't think there's a lot of value in defining a constant otherwise, since it isn't the definitive version. -- 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: 2 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, 24 Jul 2018 22:15:19 +0000 Gerrit-HasComments: Yes
