Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17917 )
Change subject: KUDU-3326 [delete]: Add soft delete table supports ...................................................................... Patch Set 81: Code-Review+1 (57 comments) Almost there: just some nits http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@700 PS81, Line 700: , the table will not be : * reserved after being deleted just do the same with C++ API. How about: The table is purged immediately. http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@727 PS81, Line 727: deleted nit: soft-deleted http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@736 PS81, Line 736: deleted nit: soft-deleted http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@744 PS81, Line 744: RecallDeletedTableRequest recall = new RecallDeletedTableRequest(this.masterTable, : id, : newTableName, : timer, : defaultAdminOperationTimeoutMs); nit: fix the alignment of the arguments http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@899 PS81, Line 899: yet nit: drop 'yet' http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@899 PS81, Line 899: normal nit: regular http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@899 PS81, Line 899: ( nit: add a space before the opening parenthesis http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@907 PS81, Line 907: normal regular http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@925 PS81, Line 925: use to decide nit: drop this part http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@925 PS81, Line 925: ( nit: add a space before the opening parenthesis http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@925 PS81, Line 925: normal regular http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@925 PS81, Line 925: yet nit: drop the 'yet' http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@926 PS81, Line 926: normal regular http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java: http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@177 PS81, Line 177: after being deleted nit: before being purged http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@202 PS81, Line 202: rename give http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@202 PS81, Line 202: with the http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@262 PS81, Line 262: normal regular http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@263 PS81, Line 263: normal regular http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@271 PS81, Line 271: normal regular http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@292 PS81, Line 292: a nit: drop 'a' http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/RecallDeletedTableRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/RecallDeletedTableRequest.java: http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/RecallDeletedTableRequest.java@76 PS81, Line 76: nit: the indent is messed up http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@210 PS81, Line 210: String newTableName = "NewTable"; nit: add 'final' ? http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client-test.cc@4988 PS81, Line 4988: nit: the indent is messed up http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client-test.cc@5015 PS81, Line 5015: nit: the indent is messed up http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client-test.cc@5097 PS81, Line 5097: Check data from table nit: Check the data in the table. http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client-test.cc@5128 PS81, Line 5128: Check data from table. Check the data in the table. http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client.h@691 PS81, Line 691: system versions http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client.h@692 PS81, Line 692: reserve reserving http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client.cc@582 PS81, Line 582: tables->clear(); nit: add tables->reserve(tables_info.size()); http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client.cc@593 PS81, Line 593: tables->clear(); nit: add tables->reserve(tables_info.size()); http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@363 PS81, Line 363: RETURN_NOT_OK(master_->catalog_manager()->SoftDeleteTableRpc( : req, &resp, nullptr)); : return Status::OK(); nit: this can be just return master_->catalog_manager()->SoftDeleteTableRpc(...); http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@373 PS81, Line 373: RETURN_NOT_OK(master_->catalog_manager()->RecallDeletedTableRpc( : req, &resp, nullptr)); : return Status::OK(); nit: this can be reduced to return master_->catalog_manager()->RecallDeletedTableRpc(...); http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3656 PS81, Line 3656: CatalogManager::TableInfoMapType::kAllTableType, : &is_soft_deleted_table, &is_expired_table)); nit: the indent is messed up, see https://google.github.io/styleguide/cppguide.html#Function_Calls for the guideline http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3667 PS81, Line 3667: CatalogManager::TableInfoMapType::kAllTableType, : &is_soft_deleted_table, &is_expired_table)); nit: the indent is messed up http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3680 PS81, Line 3680: CatalogManager::TableInfoMapType::kAllTableType, : &is_soft_deleted_table, &is_expired_table)); nit: the indent is messed up http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3708 PS81, Line 3708: CatalogManager::TableInfoMapType::kAllTableType, : &is_soft_deleted_table, &is_expired_table)); nit: the indent is messed up http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3719 PS81, Line 3719: CatalogManager::TableInfoMapType::kAllTableType, : &is_soft_deleted_table, &is_expired_table)); nit: the indent is messed up http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3732 PS81, Line 3732: CatalogManager::TableInfoMapType::kAllTableType, : &is_soft_deleted_table, &is_expired_table)); nit: the indent is messed up http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.h File src/kudu/master/master.h: http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.h@168 PS81, Line 168: to delete expired reserved tables ... to purge soft-deleted tables with expired reservations. http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@103 PS81, Line 103: " nit: missing space before the closing quote http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@103 PS81, Line 103: seconds (in seconds) http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@104 PS81, Line 104: is expired, with expired reservation period. http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@104 PS81, Line 104: this kind of table will be deleted and can not be " : "recalled later. Such tables will be purged and cannot be recalled after that. http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@289 PS81, Line 289: is not nit: are not http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@484 PS81, Line 484: CatalogManager::TableInfoMapType::kSoftDeletedTableType, : &is_soft_deleted_table, &is_expired_table); nit: the indent is messed up http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@498 PS81, Line 498: $0", table.name() Could you please add table_id into the output as well? It's useful to have table_id when doing post-mortem analysis with only logs on hand. http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.proto@624 PS81, Line 624: normal regular http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc@4570 PS81, Line 4570: client->ListSoftDeletedTables(&kudu_tables) Wrap this call into ASSERT_OK() http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc@4573 PS81, Line 4573: client->ListTables(&kudu_tables) Wrap this call into ASSERT_OK() http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc@4580 PS81, Line 4580: client->ListTables(&kudu_tables) Wrap this call into ASSERT_OK() http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc@4588 PS81, Line 4588: client->ListTables(&kudu_tables) Wrap this call into ASSERT_OK() http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc@4589 PS81, Line 4589: ASSERT_EQ(1, kudu_tables.size()); In addition to checking the size of the list, does it make sense to check for the new name of the recalled table? http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc@4591 PS81, Line 4591: client->ListSoftDeletedTables(&kudu_tables) Wrap this call into ASSERT_OK() http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/tool_action_table.cc@144 PS81, Line 144: show_soft_deleted Let's rename this into 'soft_deleted_only'. Otherwise, the 'show_soft_deleted' hints that _both_ the regular and soft-deleted are to be shown. http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/tool_action_table.cc@145 PS81, Line 145: normal regular http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/tool_action_table.cc@145 PS81, Line 145: Show soft_deleted Show only soft-deleted ... http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/tool_action_table.cc@175 PS81, Line 175: after being soft-deleted before purging a soft-deleted table -- 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: 81 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[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: Fri, 12 Aug 2022 05:36:03 +0000 Gerrit-HasComments: Yes
