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 5: (6 comments) I took a quick look and found that some comments from my first review haven't been addressed. Please take a look and address them. Thank you! 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@126 PS54, Line 126: using master::DeleteTableResponsePB; : using master::GetTableSchemaRequestPB; : using master::GetTableSchemaResponsePB; : using master::IsAlterTableDoneRequestPB; : using master::IsAlterTableDoneResponsePB; : using master::IsCreateTableDoneRequestPB; : using master::IsCreateTableDoneResponsePB; : using master::ListTabletServersResponsePB; : using master::ListTabletServersRequestPB; : using master::RecallDeletedTableRequestPB; : using master::RecallDeletedTableResponsePB; : using master::MasterFeatures; : using master::MasterServiceProxy; : using master::TableIdentifierPB; : using rpc::BackoffType; : using rpc::CredentialsPolicy; : using security::SignedTokenPB; : using strings::Substitute; : : namespace client { : : Status RetryFunc(const MonoTime& deadline, : const string& retry_msg, : const str Move this out of the namespace's scope, joining with the using block above. 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: /// @endcond : : /// Create a KuduTableAlterer object. : /// Are you sure this change is ABI-compatible? See https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B for details. 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: bool force_on_trashed_table = false, : uint32_t reserve_seconds = 0); > I guess this change will break ABI compatibility: applications build agains This comment hasn't been addressed http://gerrit.cloudera.org:8080/#/c/17917/5/src/kudu/client/client.h@714 PS5, Line 714: /// Recall a deleted but still reserved table. : /// : /// @param [in] table_name : /// Name of the table to recall. : /// @return Operation status. : : Status RecallTable(const std::string& table_name); > The main reason for designing this interface is in some scenarios?tables ma OK, if your current design allows a table with the same name to be created and then soft-deleted multiple times, I'd like to see a test case for that implemented. As for the parameter to restore a table to, I think it would be great to have an option to restore the table under different name. Imagine a case when there is already a table with the original name and it's in use by the applications, so you don't want to disrupt those, but there is also a thrashed table you want to recall, and the grace period for the soft-deleted table is expiring. What do you do to save your precious data from the soft-deleted table then? 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: RETURN_NOT_OK(AlterTableRpc(alter_req, &alter_resp, rpc, false)); If this fails, should the output RecallDeletedTableResponsePB parameter get populated with proper information on the error as well? 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: WaitUntil(MonoTime::Now() + kWait) Consider using WaitFor(kWait) instead. -- 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: 5 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:00:02 +0000 Gerrit-HasComments: Yes
