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 56: (11 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? Yes, used to set default parameters in function DeleteTable. http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client-internal.cc@126 PS54, Line 126: } // namespace security : : namespace client { : : Status RetryFunc(const MonoTime& deadline, : const string& retry_msg, : const string& timeout_msg, : const std::function<Status(const MonoTime&, bool*)>& func) { : DCHECK(deadline.Initialized()); : : if (deadline < MonoTime::Now()) { : return Status::TimedOut(timeout_msg); : } : : double wait_secs = 0.001; : const double kMaxSleepSecs = 2; : while (1) { : MonoTime func_stime = MonoTime::Now(); : bool retry = true; : Status s = func(deadline, &retry); : if (!retry) { : return s; : } : MonoTime now = MonoTim > Move this out of the namespace's scope, joining with the using block above. Done 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 If it's a function code, I think it can be disassembled. However, this function is required for the unit test added for the current modification. 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; > Whoops, sorry -- this method isn't exported, so there is no need to preserv Done 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_id : /// ID of the table to recall. : /// @param [in] new_table_nam > This doc seems outdated, needs an update. Done http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client.h@772 PS54, Line 772: stTabletServers(std::vector<Kud > nit: duplication of 'tables' in the description. Done http://gerrit.cloudera.org:8080/#/c/17917/54/src/kudu/client/client.h@783 PS54, Line 783: const std::string& fi > nit: duplication of 'tables' in the description. Done http://gerrit.cloudera.org:8080/#/c/17917/5/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/17917/5/src/kudu/client/client.h@690 PS5, Line 690: /// @return Operation status. : Status DeleteTable(const std::string& table_name) > This comment hasn't been addressed Sorry, I forgot to reply. This has been modified in the latest code.Because a version was rewritten after the discussion. https://issues.apache.org/jira/browse/KUDU-3326 Could you help to review the latest version of the code? http://gerrit.cloudera.org:8080/#/c/17917/5/src/kudu/client/client.h@714 PS5, Line 714: /// @param [in] modify_external_catalogs : /// Whether to apply the deletion to external catalogs, such as the Hive Metastore, : /// which the Kudu master has been configured to integrate with. : /// @param [in] force_on_trashed_table : /// Whether to force to delete a trashed table. : /// @param [in] reserve_seconds : /// Reserve seconds after being deleted. > OK, if your current design allows a table with the same name to be created Allowing the creation of the same table name as the trash table is outdated. In the latest discussion with Andrew, this setting is no longer allowed. http://gerrit.cloudera.org:8080/#/c/17917/8/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17917/8/src/kudu/master/catalog_manager.cc@2493 PS8, Line 2493: if (hms_catalog_ && req.modify_external_catalogs()) { > If this fails, should the output RecallDeletedTableResponsePB parameter get Done http://gerrit.cloudera.org:8080/#/c/17917/8/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/17917/8/src/kudu/master/master.cc@449 PS8, Line 449: ted tables. > Consider using WaitFor(kWait) instead. 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: 56 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: Tue, 19 Apr 2022 09:52:59 +0000 Gerrit-HasComments: Yes
