Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 )
Change subject: KUDU-2191: Metadata Upgrade Tool ...................................................................... Patch Set 13: (10 comments) http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc File src/kudu/hms/hms_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc@237 PS12, Line 237: table.__set_parameters({ > I think it would be better to remove this behavior from the KuduMetastorePl Done http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc@456 PS12, Line 456: hms_tables_map, kExternalTableName)); > instead of calling GetTable and passing in the parameter, perhaps it's bett Done http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@91 PS12, Line 91: const std::string& db_name, > This is the name of the table as stored in the HMS, right? If so, it might Done http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@96 PS12, Line 96: or a > nit: 'is the legacy...' Done http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@100 PS12, Line 100: // This method will fail if the HMS is unreachable. > I'm not quite following this bit - why is storage_handler taken as an argum In the following patch, the same method is used to retrieve non-legacy tables. So it takes storage_handler as an argument. The reason why the need to have a map is to avoid O(n) table lookup given a Kudu table's name. http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.cc@193 PS12, Line 193: > nit: 'non-legacy' Done http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool.proto@43 PS12, Line 43: // Whether or not to create a Hive Metastore, and/or enable Kudu Hive : // Metastore integration. : optional HmsMode hms_mode = 7; : : // The directory where the cluster's data and l > Same feedback. Done http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@67 PS12, Line 67: non-Impala > 'non-Impala' here and below (non should usually be hyphen separated from th Done http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@136 PS12, Line 136: > This should probably go before creating the HmsCatalog. Done http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@145 PS12, Line 145: .master_server_addrs(master_addresses) > Maybe add a comment here? I'm not sure why this would need to have non-def Oops, thanks for pointing it out. I think actually it is not needed. -- 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: 13 Gerrit-Owner: Hao Hao <[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-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 11 May 2018 01:50:06 +0000 Gerrit-HasComments: Yes
