[kudu-CR] KUDU-3326 [delete]: Add soft delete table supports

["Yingchun Lai (Code Review)" <ge...@cloudera.org>]

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 <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 06 Apr 2022 03:18:22 +0000
Gerrit-HasComments: Yes