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 78: (16 comments) http://gerrit.cloudera.org:8080/#/c/17917/75/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java: http://gerrit.cloudera.org:8080/#/c/17917/75/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java@144 PS75, Line 144: client.deleteTable(TABLE_NAME).join(); > Since the old version 'deleteTable' will delete the table immediately, so i Done http://gerrit.cloudera.org:8080/#/c/17917/75/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java: http://gerrit.cloudera.org:8080/#/c/17917/75/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@342 PS75, Line 342: assertEquals(0, asyncClient.getTablesList(NON_EXISTENT_TABLE) > Same, you can use the old version of getTablesList (with 1 parameter) Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/client/client.cc@591 PS75, Line 591: > nit: better to add some comments (e.g. '/*list_tablet_with_partition=*/ tru Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/integration-tests/master_hms-itest.cc@484 PS75, Line 484: TEST_F(MasterHmsTest, TestSoftDeleteTable) { > Add a TODO to remind us to complete the remain work in the future. Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/integration-tests/master_hms-itest.cc@500 PS75, Line 500: ASSERT_OK(harness_.hms_client()->GetTable("default", "a", &hms_table)); > nit: hms_client only indicates that the table exists in HMS, we'd better al Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@2241 PS75, Line 2241: TableInfoMapType > nit: Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@2389 PS75, Line 2389: if (s.ok() && is_soft_deleted_table && req.reserve_seconds() != 0) { > It's confused why not return error when 's' is not OK. We will look for the corresponding table in the map. The data may be corrupted and the table cannot be found. So i make it return 'Status'. http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@2408 PS75, Line 2408: leader_lock_.AssertAcquiredForReading(); : > An log has been added to SoftDeleteTableRpc, it's duplicated here. Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@2416 PS75, Line 2416: > Better to add some like: " when HMS is enabled." Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@2707 PS75, Line 2707: if (s.ok() && !(is_soft_deleted_table || is_expired_table)) { > An log has been added to RecallDeletedTableRpc, it's duplicated here. Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@3205 PS75, Line 3205: // rename the table in the Kudu catalog. Instead, rename the table > An log has been added above, it's duplicated here. Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@6686 PS75, Line 6686: } > nit: alignment Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@6712 PS75, Line 6712: } > nit: you can move it out of the lock. Done http://gerrit.cloudera.org:8080/#/c/17917/76/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17917/76/src/kudu/master/catalog_manager.cc@6723 PS76, Line 6723: fo> table_by_i > nit: the function name seems not so suitable to what it actullay do. Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/master.cc@290 PS75, Line 290: > Now the HMS enable status check is at runtime, can we check it when bootstr Done http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/master_service.cc@584 PS75, Line 584: > nit: add " when HMS is enabled." 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: 78 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: Tue, 09 Aug 2022 08:09:08 +0000 Gerrit-HasComments: Yes
