Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17917 )
Change subject: KUDU-3326 [delete]: Add soft delete table supports ...................................................................... Patch Set 47: (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? http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@403 PS47, Line 403: DECLARE_int32(max_priority_range); What is it used for? http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@2227 PS47, Line 2227: scoped_refptr<TableInfo> CatalogManager::FindTableWithName(const string& table_name, nit: Add 'Unlocked' postfix to indicate that this function must be called in lock. http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@3978 PS47, Line 3978: *exists = ContainsKey(normalized_table_names_map_, NormalizeTableName(table_name)) Reuse CatalogManager::FindTableWithName? http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@6578 PS47, Line 6578: trash_table_names_map_[table_name] = table; Is it possible that there is a same table name in trash_table_names_map_? If it's impossible, better to add a CHECK. http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@6586 PS47, Line 6586: auto table = FindPtrOrNull(table_ids_map_, req.table().table_id()); I'm confused why check it must be in table_ids_map_, but not trash_table_names_map_? http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@6597 PS47, Line 6597: normalized_table_names_map_[table_name] = table; Same, better to add a CHECK to say it's not in normalized_table_names_map_. http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@6743 PS47, Line 6743: INITTED_AND_LEADER_OR_RESPOND(RecallDeletedTableResponsePB); nit: keep in lexicographical order. http://gerrit.cloudera.org:8080/#/c/17917/47/src/kudu/master/catalog_manager.cc@6873 PS47, Line 6873: return Substitute("$0", l.data().pb.name()); Return 'l.data().pb.name()' directly is OK? 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. 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 RecallDeletedTableRequestPB; nit: better to keep them in lexicographical order. 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_trash=true. 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: NO_FATALS(RunActionStdoutNone(Substitute("table recall $0 $1 --new_table_name=$2", Better to test how it act if there is a same table name exist. 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(rename_column)) nit: Better to keep them in lexicographical order. -- 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: 47 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: Wed, 06 Apr 2022 03:18:22 +0000 Gerrit-HasComments: Yes
