You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2022/05/09 17:29:56 UTC
[kudu-CR] KUDU-3326 [delete]: Add soft delete table supports
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 <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 09 May 2022 17:29:56 +0000
Gerrit-HasComments: Yes