Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 )
Change subject: KUDU-2191: Metadata Upgrade Tool ...................................................................... Patch Set 19: Verified+1 (9 comments) Unrelated flaky test failure. http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h@21 PS16, Line 21: #include <vector> > looks like this is no longer needed? Curious that IWYU didn't catch this. Yeah, it looks like this file is filtered to not run IWYU?https://github.com/apache/kudu/blob/master/build-support/iwyu.py#L65 http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h@103 PS16, Line 103: SL gflags. > I'd drop this last part of the sentence, I think it's clear from the signat Done http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@75 PS16, Line 75: "table should reside in"; > Might be simpler to return the map instead of taking it as an out param. Done http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@121 PS16, Line 121: bool exist; > Check the return status. Done http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@123 PS16, Line 123: if (!exist) { > This could cause problems for installations which have two Kudu clusters po Done http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@135 PS16, Line 135: > What do you think about re-organizing this so that the failure to upgrade a Done http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@146 PS16, Line 146: if (hms_table->parameters[HmsClient::kStorageHandlerKey] == > Maybe worth doing a check that 'new_table_name' isn't contained in table_na Done http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@148 PS16, Line 148: string new_table_name = Substitute("$0.$1", hms_table->dbName, hms_table->tableName); > Since this will probably land before the listener, just add a TODO for me t Done http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@156 PS16, Line 156: client::SchemaFromKuduSchema(kudu_table->schema())); > As we discussed offline, I think we should just assume the table has alread Done -- To view, visit http://gerrit.cloudera.org:8080/10075 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3 Gerrit-Change-Number: 10075 Gerrit-PatchSet: 19 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 18 May 2018 18:29:43 +0000 Gerrit-HasComments: Yes
