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 76: (17 comments) http://gerrit.cloudera.org:8080/#/c/17917/76/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/76/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@398 PS76, Line 398: private final int defaultSoftDeletedTableReservedSeconds; Where is this variable used? Seems the parameter is passed manually every time by deleteTable. 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, 0).join(); Since the old version 'deleteTable' will delete the table immediately, so it's not needed to specify an extra '0' parameter. (Other tests are the same.) 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, false) Same, you can use the old version of getTablesList (with 1 parameter) 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: true, true nit: better to add some comments (e.g. '/*list_tablet_with_partition=*/ true, /*show_soft_deleted=*/ true') for boolean type parameter, it will be more readable for developers. 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. http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/integration-tests/master_hms-itest.cc@500 PS75, Line 500: } nit: hms_client only indicates that the table exists in HMS, we'd better also check that the "default" table is still remain in the Kudu cluster? 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: How abour define TableInfoMapType as: enum TableInfoMapType { kNormalTableType = 1 << 0, kSoftDeletedTableType = 1 << 1, kAllTableType = 0b00000011, }; Then you can avoid one lookup if not kAllTableType, by judging if the bit set. 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. http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@2408 PS75, Line 2408: LOG(INFO) << Substitute("Servicing SoftDeleteTable request from $0:\n$1", : RequestorString(rpc), SecureShortDebugString(req)); An log has been added to SoftDeleteTableRpc, it's duplicated here. 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." http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@2707 PS75, Line 2707: LOG(INFO) << Substitute("Servicing RecallDeletedTable request from $0:\n$1", An log has been added to RecallDeletedTableRpc, it's duplicated here. http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@3205 PS75, Line 3205: LOG(INFO) << Substitute("Servicing AlterTable request from $0:\n$1", An log has been added above, it's duplicated here. http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@6686 PS75, Line 6686: NormalizeTableName(table_name)); nit: alignment http://gerrit.cloudera.org:8080/#/c/17917/75/src/kudu/master/catalog_manager.cc@6712 PS75, Line 6712: const string table_name = table->table_name(); nit: you can move it out of the lock. 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: IsTableExpired nit: the function name seems not so suitable to what it actullay do. 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: if (!hms::HmsCatalog::IsEnabled()) { Now the HMS enable status check is at runtime, can we check it when bootstrap? 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." -- 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: 76 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: Mon, 08 Aug 2022 13:03:22 +0000 Gerrit-HasComments: Yes
