Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 )
Change subject: KUDU-2191: Metadata Upgrade Tool ...................................................................... Patch Set 12: (9 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: // Do not add other parameters such as 'kudu.master_addresses' since I think it would be better to remove this behavior from the KuduMetastorePlugin than work around it here, since the workaround means we might miss test coverage of some case where that property applies. http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc@456 PS12, Line 456: table.parameters[HmsClient::kLegacyKuduTableNameKey] instead of calling GetTable and passing in the parameter, perhaps it's better to pass in kManagedTableName and kExternalTableName directly, since that's really what should be tested? 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& name, This is the name of the table as stored in the HMS, right? If so, it might be clearer to take the database name and table name separately. http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@96 PS12, Line 96: s leg nit: 'is the legacy...' http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@100 PS12, Line 100: Status RetrieveTables(const char* storage_handler, I'm not quite following this bit - why is storage_handler taken as an argument if the method only does anything when the legacy storage handler is specified? Why not just a 'RetrieveLegacyTables' method? And at that point you don't really need a map for the output type, it can just return a vector<hive::Table>. 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: on lega nit: 'non-legacy' 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 the word it's modifying) http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@136 PS12, Line 136: if (!hms::HmsCatalog::IsEnabled()) { This should probably go before creating the HmsCatalog. http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@145 PS12, Line 145: ReplicaController::SetVisibility(&builder, ReplicaController::Visibility::ALL); Maybe add a comment here? I'm not sure why this would need to have non-default replica visibility. -- 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: 12 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: Tue, 08 May 2018 23:09:27 +0000 Gerrit-HasComments: Yes
