Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13314 )
Change subject: hms: don't overwrite table type when updating metadata ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/hms/hms_catalog.cc@200 PS2, Line 200: EraseKeyReturnValuePtr(&table.parameters, HmsClient::kExternalTableKey); See other feedback. http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/hms/hms_catalog.cc@211 PS2, Line 211: EraseKeyReturnValuePtr(&table.parameters, HmsClient::kKuduTableIdKey); See other feedback. http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/hms/hms_catalog.cc@361 PS2, Line 361: This can lead to races between concurrent DROP : // TABLE and CREATE TABLE operations. I hinted at this in the previous review, but how concerned should I be about this? Does this mean concurrent DROP/CREATE TABLE ops for the same table are now going to fail? What's the effect for Kudu users? http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/hms/hms_catalog.cc@365 PS2, Line 365: EraseKeyReturnValuePtr(&table->parameters, HmsClient::kExternalTableKey); This is more correct than setting it to "FALSE"? BTW, I think table->parameters.erase(...) is clearer; the "ReturnValuePtr" part of the call has no bearing here. http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/tools/tool_action_hms.cc@168 PS2, Line 168: HmsTableType table_type = hms_table.tableType == HmsClient::kManagedTable ? : HmsTableType::MANAGED : HmsTableType::EXTERNAL; By my count this is the third time that the pattern of "get an existing hive::Table, pull out its table type, convert it into HmsTableType, and pass that into PopulateTable" has occurred. Maybe we need another HmsTableType option for "passthrough" or some such, which means just preserve the existing table type in the hive::Table? -- To view, visit http://gerrit.cloudera.org:8080/13314 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If1b658ccb1186e4a238567b08dfbc00b0f2244f6 Gerrit-Change-Number: 13314 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 11 May 2019 21:09:58 +0000 Gerrit-HasComments: Yes
