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 57: (26 comments) Thank you for working on this useful feature! I took another quick look. I hope to have another look this week so don't rush to address everything I mentioned yet, but it's worth discussing high-level questions in the comments on gerrit. Thank you very much! http://gerrit.cloudera.org:8080/#/c/17917/57/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/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@305 PS57, Line 305: DEFAULT_TRASHTABLE_RESERVED_SECONDS nit: please separate TRASH and TABLE with underscore DEFAULT_TRASH_TABLE_RESERVED_SECONDS http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@702 PS57, Line 702: /** : * SoftDelete a table on the cluster with the specified name, the table will be : * reserved for 7 days after being deleted. : * @param name the table's name : * @return a deferred object to track the progress of the deleteTable command : */ : public Deferred<DeleteTableResponse> deleteTable(String name) { : return deleteTable(name, false, this.defaultTrashTableReservedSeconds); : } With this, it seems there is a discrepancy in behavior of the Java and C++ clients: the C++ API maintains the legacy behavior, while Java client now by default does soft-deletion. We strive to keep the behaviors in sync, so I guess Java client API should behave here the same a C++ API does: drop the table without any reservations. http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@713 PS57, Line 713: on the cluster nit here and below: drop this 'on the cluster' since it's self-evident all the changes are on the current cluster http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@905 PS57, Line 905: normal nit: normal (i.e. not yet thrashed) http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@918 PS57, Line 918: showTrash Please document this new parameter. http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@929 PS57, Line 929: all the tables all the thrashed tables http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1171 PS57, Line 1171: trash table a thrashed table http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2832 PS57, Line 2832: DEFAULT_TRASHTABLE_RESERVED_SECONDS DEFAULT_TRASH_TABLE_RESERVED_SECONDS http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2915 PS57, Line 2915: : nit: please remove the extra line http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2919 PS57, Line 2919: A value of 0 disables the reserved trash table and delete table directly. nit: how about A value of 0 disables the soft-delete behavior, so the thrashed table is not kept around and purged immediately. http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client-internal.h@257 PS57, Line 257: static const uint32_t kTrashTableReservedSeconds = 60 * 60 * 24 * 7; Style: move this to the very beginning of the 'public' section in this class. 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) > OK?I'll take this part out right away. Thanks a lot! http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client-test.cc@4956 PS57, Line 4956: remove removed http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client-test.cc@4956 PS57, Line 4956: to tarsh map from the trash map http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client-test.cc@4965 PS57, Line 4965: Alter trashed table Altering a thrashed table http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client-test.cc@5016 PS57, Line 5016: create creating http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client.h@685 PS57, Line 685: /// In order to be compatible with the old version, will : /// call the SoftDeleteTable directly. Drop this part: this is a description of the interface, and it's not necessarily to disclose what it does in the implementation. http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client.h@694 PS57, Line 694: /// After this short header, it would be great to add more details on what SoftDeleteTable() actually does. Please see the comment for SetVerboseLogLevel() for example. The idea is to provide the essence what 'soft delete' means and refer to other related methods which are relevant for the use-cases involving soft-deleting and recalling tables. http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client.h@732 PS57, Line 732: New table name for the recalled table. Please describe the semantics/meaning of the empty string (i.e. "") for the 'new_table_name' parameter. http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client.h@1070 PS57, Line 1070: Status GetTables(std::vector<std::string>* tables, const std::string& filter, : bool show_trash); Why to have this method in here instead of client_internal.h? As far as I can see, this method is used only in client.cc, so making it a part of the KuduClient::Data() looks like a better choice. http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/common/wire_protocol.cc@26 PS57, Line 26: #include <unordered_set> : #include <string> nit: why is this change? The original order of includes was correct: in each section, the headers should be sorted alphabetically. http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/common/wire_protocol.cc@72 PS57, Line 72: using std::unordered_set; : using std::string; nit: why is this change? The original order was correct. http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/master/master.proto@94 PS57, Line 94: TABLE_NOT_NORMAL Maybe, name this 'TABLE_SOFT_DELETED' to be more precise? http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/master/master.proto@588 PS57, Line 588: // Reserve seconds after the table has been deleted. : optional uint32 reserve_seconds = 3; : : // Force to delete a trashed table. : optional bool force_on_trashed_table = 4 [default = false]; What if we remove the 'force_on_trashed_table' field and keep only the 'reserve_seconds' field with the semantics of reserve_seconds==0 meaning purging the table without any grace period? http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/master/master.proto@603 PS57, Line 603: If not null, set table name with this field for the recalled table. how about: If this field is set, that's the name for the recalled table. http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/master/master_service.cc@594 PS57, Line 594: Status s = server_->catalog_manager()->AlterTableRpc( : *req, resp, rpc); nit: why is this change? -- 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: 57 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, 09 May 2022 17:29:56 +0000 Gerrit-HasComments: Yes
