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 70: -Code-Review (27 comments) Thanks a lot for continuing work on this! 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: DELTED DELETED http://gerrit.cloudera.org:8080/#/c/17917/70/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@710 PS70, Line 710: DELTED DELETED http://gerrit.cloudera.org:8080/#/c/17917/70/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@906 PS70, Line 906: normal(i.e. not yet thrashed) normal (i.e. not soft-deleted) http://gerrit.cloudera.org:8080/#/c/17917/70/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@921 PS70, Line 921: public Deferred<ListTablesResponse> getTablesList(String nameFilter, boolean showSoftDeleted) { I didn't notice this during previous review rounds, but this is a public method and changing its signature breaks the backward compatibility of the Kudu Java API, right? In other words, old applications using getTablesList() would require changing their code and recompilation to work with newer kudu-client JAR. That's not acceptable. Please either introduce a new method or add 'nameFilter' parameter for getSoftDeletedTablesList() and combine lists retrieved by getTablesList() and updated getSoftDeletedTablesList() to get the list of all tables in the system. Probably, there are other options as well. http://gerrit.cloudera.org:8080/#/c/17917/70/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2922 PS70, Line 2922: so the thrashed table is not kept : * around and purged immediately so the table is purged immediately 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, false, 0); client.deleteTable(TABLE_NAME) is more concise and behaves the same now, right? Then maybe return the original client.deleteTable(TABLE_NAME) back. 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, false, 0) It's enough to have client.deleteTable(tableName) here, right? 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: thrashed nit: soft-deleted 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-zero reservation period and force=false on the soft-deleted table to be explicit about the expected behavior. 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 to add a sub-scenario trying to drop the soft-deleted table to be explicit about the expected behavior. http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/client/client-test.cc@5055 PS70, Line 5055: nit: wrong indent http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/client/client-test.cc@5126 PS70, Line 5126: // Check data from table. nit: just for sanity check and to be explicit about the expected behavior, could you add an assert on table_id after recalling the table back? ASSERT_EQ(table_id, client_table_->id()); 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_STR_CONTAINS(s.ToString(), "RecallDeletedTable is not supported"); Do you mind adding an assert for the type of the status code returned by client_->RecallTable(), something like you did at line 493? Thanks! ASSERT_TRUE(s.IsNotSupported()) << s.ToString(); 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: uint64_t To avoid underflow in some edge-case scenarios and improve the readability a bit, change the condition into pb.delete_timestamp() + pb.soft_deleted_reserved_seconds() <= static_cast<uint64_t>(WallTime_Now()) http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.h@271 PS70, Line 271: ( readability nit: drop the enclosing/outer parentheses -- they are not needed 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: nit: wrong indent -- should be +4 spaces (not +2) from the previous line http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@593 PS70, Line 593: normal What's a "normal" table name? And why to mention that at all when in case of soft-deleted table the behavior is the same? 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 not" nit: missing space after 'not' http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@608 PS70, Line 608: "differ only by case; restart the master(s) with the Hive Metastore integration disabled" nit: missing space after 'disabled' http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@2390 PS70, Line 2390: ( readability nit: drop these parentheses -- they are not needed http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@2390 PS70, Line 2390: is_soft_deleted_table && !req.force_on_soft_deleted_table() I'm still trying to understand (without much success, though): what does this 'force_on_soft_deleted_table' field buys you? In other words, why not just treat req.reserve_seconds() == 0 as a request to drop/delete the table regardless whether it's soft-deleted or not (that's exactly how it's treated later at line 2398)? Thanks! http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/master/catalog_manager.cc@2392 PS70, Line 2392: nit: wrong indent -- should +4 spaces from the beginning of the original line above 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: const string& nit: at line 4458 that's just 'const string', maybe make these both to be compile-time constants by changing to 'constexpr const char* const'? http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/tools/kudu-tool-test.cc@4546 PS70, Line 4546: ASSERT_EQ nit for here and elsewhere with ASSERT_EQ: please put the expected/reference value first, otherwise it's hard to read error messages if those asserts trigger. I.e., in this case it supposed to be ASSERT_EQ(1, kudu_tables.size()); 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 name of the recalled table how about: The new name for the recalled table. http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/tools/tool_action_table.cc@116 PS70, Line 116: If the empty string, use the same name as before deletion. how about: Leave empty to recall the table under its original name. http://gerrit.cloudera.org:8080/#/c/17917/70/src/kudu/tools/tool_action_table.cc@139 PS70, Line 139: after being deleted after table is soft-deleted -- 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: 70 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: Sun, 24 Jul 2022 00:47:05 +0000 Gerrit-HasComments: Yes
