KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/17917 )
Change subject: KUDU-3326 [delete]: Add soft delete table supports ...................................................................... Patch Set 50: (14 comments) http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@a3207 PS47, Line 3207: > Is it related to this patch? Done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@403 PS47, Line 403: > What is it used for? done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@2227 PS47, Line 2227: const string& table_name, > nit: Add 'Unlocked' postfix to indicate that this function must be called i Done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@3978 PS47, Line 3978: leader_lock_.AssertAcquiredForReading(); > Reuse CatalogManager::FindTableWithName? Done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@6578 PS47, Line 6578: table_name)); > Is it possible that there is a same table name in trash_table_names_map_? I Done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@6586 PS47, Line 6586: Status CatalogManager::MoveToNormalContainer(const RecallDeletedTableRequestPB& req) { > I'm confused why check it must be in table_ids_map_, but not trash_table_na In the previous discussion, it was determined that we need to recall with table ID. https://issues.apache.org/jira/browse/KUDU-3326 http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@6597 PS47, Line 6597: if (trash_table_names_map_.erase(NormalizeTableName(table_name)) != 1) { > Same, better to add a CHECK to say it's not in normalized_table_names_map_. Done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@6743 PS47, Line 6743: INITTED_AND_LEADER_OR_RESPOND(AddMasterResponsePB); > nit: keep in lexicographical order. Done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@6873 PS47, Line 6873: return Substitute("$0 [id=$1]", l.data().pb.name(), table_id_); > Return 'l.data().pb.name()' directly is OK? Done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/master.proto@622 PS47, Line 622: // Only show normal tables if false. > nit: remove duplicate spaces. Done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/master_service.h File src/kudu/master/master_service.h: http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/master_service.h@76 PS47, Line 76: class RefreshAuthzCacheRequestPB; > nit: better to keep them in lexicographical order. Done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/tools/kudu-admin-test.cc@1678 PS47, Line 1678: > Add some test cases to check that trashed tables can be list when -show_tra Done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/tools/kudu-tool-test.cc@4361 PS47, Line 4361: workload.set_table_name(kTableName); > Better to test how it act if there is a same table name exist. Done http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/tools/tool_action_table.cc@1781 PS47, Line 1781: .AddAction(std::move(recall)) > nit: Better to keep them in lexicographical order. Done -- To view, visit http://gerrit.cloudera.org:8080/17917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d1dddfbca55a5c4bcac4028157325ad618ea665 Gerrit-Change-Number: 17917 Gerrit-PatchSet: 50 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Thu, 07 Apr 2022 03:00:19 +0000 Gerrit-HasComments: Yes
