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 71: (27 comments) http://gerrit.cloudera.org:8080/#/c/17917/70/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/70/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@306 PS70, Line 306: DELETE > DELETED Done http://gerrit.cloudera.org:8080/#/c/17917/70/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@710 PS70, Line 710: _STATE > DELETED Done http://gerrit.cloudera.org:8080/#/c/17917/70/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@906 PS70, Line 906: esponse> getTablesList() { > normal (i.e. not soft-deleted) Done http://gerrit.cloudera.org:8080/#/c/17917/70/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@921 PS70, Line 921: defaultAdminOperationTimeoutMs); > I didn't notice this during previous review rounds, but this is a public me Done http://gerrit.cloudera.org:8080/#/c/17917/70/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2922 PS70, Line 2922: : * @return this builder > so the table is purged immediately Done http://gerrit.cloudera.org:8080/#/c/17917/70/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/70/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@153 PS70, Line 153: client.deleteTable(TABLE_NAME); > client.deleteTable(TABLE_NAME) is more concise and behaves the same now, ri Done http://gerrit.cloudera.org:8080/#/c/17917/70/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/70/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@225 PS70, Line 225: client.deleteTable(tableName); > It's enough to have client.deleteTable(tableName) here, right? Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/client/client-test.cc@4999 PS70, Line 4999: soft-del > nit: soft-deleted Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/client/client-test.cc@5024 PS70, Line 5024: SoftDeleteTable(kTableName, > It would be nice to add a sub-scenario to call soft-delete with some non-ze Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/client/client-test.cc@5029 PS70, Line 5029: > Is it still allowed to drop/delete a soft-deleted table? It would be great Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/client/client-test.cc@5055 PS70, Line 5055: kTableName)); > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/client/client-test.cc@5126 PS70, Line 5126: ASSERT_EQ(old_table_id, client_table_->id()); > nit: just for sanity check and to be explicit about the expected behavior, Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/integration-tests/master_hms-itest.cc@496 PS70, Line 496: ASSERT_TRUE(s.IsNotSupported()) << s.ToString(); > Do you mind adding an assert for the type of the status code returned by cl Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.h@271 PS70, Line 271: estamp() > To avoid underflow in some edge-case scenarios and improve the readability Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.h@271 PS70, Line 271: p > readability nit: drop the enclosing/outer parentheses -- they are not neede Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@593 PS70, Line 593: u tabl > What's a "normal" table name? And why to mention that at all when in case Oh it's my fault. I will delete it because it's outdated info. http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@593 PS70, Line 593: > nit: wrong indent -- should be +4 spaces (not +2) from the previous line Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@607 PS70, Line 607: "when the Hive Metastore integration is enabled, Kudu soft-deleted table names must " > nit: missing space after 'not' Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@608 PS70, Line 608: "not differ only by case; restart the master(s) with the Hive Metastore integration " > nit: missing space after 'disabled' Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@2390 PS70, Line 2390: s_soft_deleted_table && req.reserve_seconds() != 0) { > I meant I understood that 'force_on_soft_deleted_table' allows to reject re I think I understand your meaning. So I have removed it from the latest code. http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@2390 PS70, Line 2390: i > readability nit: drop these parentheses -- they are not needed Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@2392 PS70, Line 2392: St > nit: wrong indent -- should +4 spaces from the beginning of the original li Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/tools/kudu-tool-test.cc@4526 PS70, Line 4526: // Create the > nit: at line 4458 that's just 'const string', maybe make these both to be c Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/tools/kudu-tool-test.cc@4546 PS70, Line 4546: NO_FATALS > nit for here and elsewhere with ASSERT_EQ: please put the expected/referenc Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/tools/tool_action_table.cc@115 PS70, Line 115: The new name for the recalled > how about: Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/tools/tool_action_table.cc@116 PS70, Line 116: Leave empty to recall the table under its original name.") > how about: Done http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/tools/tool_action_table.cc@139 PS70, Line 139: after being soft-de > after table is soft-deleted 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: 71 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, 26 Jul 2022 07:03:10 +0000 Gerrit-HasComments: Yes
