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 82: (56 comments) 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 is purged immediately. : * @param name the table's name > How about: Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@727 PS81, Line 727: the ta > nit: soft-deleted Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@736 PS81, Line 736: the ta > nit: soft-deleted Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@744 PS81, Line 744: this.masterTable, : id, : newTableName, : timer, : defaultAdminOperationTimeoutMs); > nit: fix the alignment of the arguments Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@899 PS81, Line 899: t s > nit: drop 'yet' Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@899 PS81, Line 899: regula > nit: regular Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@899 PS81, Line 899: r > nit: add a space before the opening parenthesis Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@907 PS81, Line 907: regula > regular Done 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: drop the 'yet' Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@925 PS81, Line 925: f > nit: add a space before the opening parenthesis Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@925 PS81, Line 925: whether to dis > nit: drop this part Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@925 PS81, Line 925: not so > regular Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@926 PS81, Line 926: regula > regular Done 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: before being purged > nit: before being purged Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@202 PS81, Line 202: give t > give Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@202 PS81, Line 202: e ne > the Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@262 PS81, Line 262: regula > regular Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@263 PS81, Line 263: regula > regular Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@271 PS81, Line 271: regula > regular Done http://gerrit.cloudera.org:8080/#/c/17917/81/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@292 PS81, Line 292: l > nit: drop 'a' Done 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: Master.RecallDeletedTableResponseP > nit: the indent is messed up Done 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: final String newTableName = "NewTable"; > nit: add 'final' ? Done 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: ASSERT_TRUE(clus > nit: the indent is messed up Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client-test.cc@5015 PS81, Line 5015: ta > nit: the indent is messed up Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client-test.cc@5097 PS81, Line 5097: > nit: Check the data in the table. Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client-test.cc@5128 PS81, Line 5128: > Check the data in the table. Done 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: versio > versions Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client.h@692 PS81, Line 692: reservi > reserving Done 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 Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/client/client.cc@593 PS81, Line 593: /*list_tablet_with_partition=*/ true, /*show_soft_deleted=*/ true)); > nit: add Done 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 master_->catalog_manager()->SoftDeleteTableRpc(req, &resp, nullptr); : } : > nit: this can be just Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@373 PS81, Line 373: : void MasterTest::DoListTables(const ListTablesRequestPB& req, ListTablesResponsePB* resp) { : RpcController contro > nit: this can be reduced to Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3656 PS81, Line 3656: } : > nit: the indent is messed up, see https://google.github.io/styleguide/cppgu Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3667 PS81, Line 3667: ASSERT_OK(RecallTable(table_id)); : } > nit: the indent is messed up Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3680 PS81, Line 3680: } : } > nit: the indent is messed up Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3708 PS81, Line 3708: } : > nit: the indent is messed up Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3719 PS81, Line 3719: ASSERT_OK(RecallTable(table_id)); : } > nit: the indent is messed up Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master-test.cc@3732 PS81, Line 3732: } : } > nit: the indent is messed up Done 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 purge soft-deleted tables with > ... to purge soft-deleted tables with expired reservations. Done 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: (in sec > (in seconds) Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@103 PS81, Line 103: t > nit: missing space before the closing quote Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@104 PS81, Line 104: with expire > with expired reservation period. Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@104 PS81, Line 104: reservation period. Such tables will be purged and cannot " : "be recalled aft > Such tables will be purged and cannot be recalled after that. Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@484 PS81, Line 484: table_identifier, : CatalogManager::TableInfoMapType::kSoftDeletedTabl > nit: the indent is messed up Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/master/master.cc@498 PS81, Line 498: esp, nullptr), > Could you please add table_id into the output as well? It's useful to have Done 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: regula > regular Done 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: workload.set_table_name(kTableName); > Wrap this call into ASSERT_OK() Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc@4573 PS81, Line 4573: > Wrap this call into ASSERT_OK() Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc@4580 PS81, Line 4580: > Wrap this call into ASSERT_OK() Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc@4588 PS81, Line 4588: NO_FATALS(StartExternalMiniClust > Wrap this call into ASSERT_OK() Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc@4589 PS81, Line 4589: shared_ptr<KuduClient> client; > In addition to checking the size of the list, does it make sense to check f Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/kudu-tool-test.cc@4591 PS81, Line 4591: string master_addr = cluster_->master()->bo > Wrap this call into ASSERT_OK() Done 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: soft_deleted_only > Let's rename this into 'soft_deleted_only'. Otherwise, the 'show_soft_dele Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/tool_action_table.cc@145 PS81, Line 145: show r > regular Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/tool_action_table.cc@145 PS81, Line 145: Show only soft-de > Show only soft-deleted ... Done http://gerrit.cloudera.org:8080/#/c/17917/81/src/kudu/tools/tool_action_table.cc@175 PS81, Line 175: before purging a soft-de > before purging a soft-deleted table 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: 82 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 09:31:23 +0000 Gerrit-HasComments: Yes
