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 58: (26 comments) 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_TRASH_TABLE_RESERVED_SECOND > nit: please separate TRASH and TABLE with underscore Done http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@702 PS57, Line 702: : /** : * Delete a table with the specified name, the table will not be : * reserved after being deleted just do the same with C++ API. : * @param name the table's name : * @return a deferred object to track the progress of the deleteTable command : */ : public Deferred<DeleteTableResponse> deleteTable(String name) { : > With this, it seems there is a discrepancy in behavior of the Java and C++ Done http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@713 PS57, Line 713: > nit here and below: drop this 'on the cluster' since it's self-evident all Done http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@905 PS57, Line 905: > nit: Done http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@918 PS57, Line 918: > Please document this new parameter. Done 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 thrashed tables Done http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1171 PS57, Line 1171: > a thrashed table Done http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2832 PS57, Line 2832: EFAULT_OPERATION_TIMEOUT_MS; > DEFAULT_TRASH_TABLE_RESERVED_SECONDS Done 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 Done http://gerrit.cloudera.org:8080/#/c/17917/57/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2919 PS57, Line 2919: Optional. > nit: how about Done 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: const security::SignedTokenPB& token); > Style: move this to the very beginning of the 'public' section in this clas 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) > Thanks a lot! Done 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: nsert > removed Done http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client-test.cc@4956 PS57, Line 4956: uccessfully > from the trash map Done http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client-test.cc@4965 PS57, Line 4965: Insert a few rows, > Altering a thrashed table Done http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client-test.cc@5016 PS57, Line 5016: sInval > creating Done 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: /// : /// The delete operation or drop opera > Drop this part: this is a description of the interface, and it's not necess Done 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 Soft Done http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client.h@732 PS57, Line 732: ivate API. > Please describe the semantics/meaning of the empty string (i.e. "") for the Done http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/client/client.h@1070 PS57, Line 1070: FRIEND_TEST(ClientTest, ClearCacheAndConcurrentWorkload); : FRIEND_TEST(ClientTest, Connection > Why to have this method in here instead of client_internal.h? As far as I Done 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 <string> : #include <unorder > nit: why is this change? The original order of includes was correct: in ea Done http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/common/wire_protocol.cc@72 PS57, Line 72: using std::string; : using std::unorder > nit: why is this change? The original order was correct. Done 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_SOFT_DELET > Maybe, name this 'TABLE_SOFT_DELETED' to be more precise? Done 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 'res Yes, you are right. The table will be deleted directly when the reserve_seconds is 0. http://gerrit.cloudera.org:8080/#/c/17917/57/src/kudu/master/master.proto@603 PS57, Line 603: If this field is set, that's the name for the recalled table. > how about: Done 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); : CheckRespErrorOrSetUnknown(s > nit: why is this change? 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: 58 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, 10 May 2022 06:51:29 +0000 Gerrit-HasComments: Yes
