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 54: (6 comments) http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client-internal.cc@73 PS54, Line 73: DEFINE_uint32(trash_table_reserved_seconds, 60 * 60 * 24 * 7, : "Default reserved time for deleted table." : "A value of 0 disables the reserved trash table and delete table directly"); Is this still used in the code at all? http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client-test.cc@715 PS54, Line 715: Status CreateTable(const string& table_name, : int num_replicas, : vector<unique_ptr<KuduPartialRow>> split_rows, : vector<pair<unique_ptr<KuduPartialRow>, : unique_ptr<KuduPartialRow>>> range_bounds, : shared_ptr<KuduTable>* table) Could you separate this change of the CreateTable() signature in a separate changelist? In general, it's a good idea to put changelist like this into their own changelists, so it's easier to focus on the essence of the newly introduced functionality in the main changelist. http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client.h@722 PS54, Line 722: Status DeleteTableInCatalogs(const std::string& table_name, : bool modify_external_catalogs, : bool force_on_trashed_table = false, : uint32_t reserve_seconds = 0) KUDU_NO_EXPORT; > Are you sure this change is ABI-compatible? Whoops, sorry -- this method isn't exported, so there is no need to preserve ABI compatibility. Please disregard my prior comment on this. http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client.h@727 PS54, Line 727: /// Recall a deleted but still reserved table. : /// : /// @param [in] table_name : /// Name of the table to recall. : /// @return Operation status. This doc seems outdated, needs an update. http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client.h@772 PS54, Line 772: normal tables only those tables nit: duplication of 'tables' in the description. http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client.h@783 PS54, Line 783: List trash tables only those tables nit: duplication of 'tables' in the description. -- 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: 54 Gerrit-Owner: KeDeng <[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, 18 Apr 2022 21:16:52 +0000 Gerrit-HasComments: Yes
