This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 528f0fad74d932cb259237d273261a7f585b6595 Author: kedeng <[email protected]> AuthorDate: Tue Sep 27 19:48:32 2022 +0800 KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC If the field 'reserve_seconds' is specified by the client, then it's clear the delete request coming from a newer Kudu client with the precise value for 'reserve_seconds', and the field's value from the request should be taken as-is regardless of the current setting of the --deleted_table_reserve_seconds flag at the server side. Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f Reviewed-on: http://gerrit.cloudera.org:8080/19047 Tested-by: Kudu Jenkins Reviewed-by: Yingchun Lai <[email protected]> --- .../org/apache/kudu/client/AsyncKuduClient.java | 23 ++++++++++------ .../org/apache/kudu/client/DeleteTableRequest.java | 14 ++++++++-- .../java/org/apache/kudu/client/KuduClient.java | 5 +++- src/kudu/client/client-internal.cc | 6 ++-- src/kudu/client/client-internal.h | 5 ++-- src/kudu/client/client-test.cc | 12 ++++++-- src/kudu/client/client.cc | 10 +++++-- src/kudu/client/client.h | 8 ++++-- src/kudu/master/catalog_manager.cc | 32 +++++++++++++++++----- src/kudu/master/master.proto | 5 ++++ src/kudu/tools/kudu-admin-test.cc | 3 +- src/kudu/tools/kudu-tool-test.cc | 5 ++-- src/kudu/tools/tool_action_table.cc | 7 +++-- 13 files changed, 97 insertions(+), 38 deletions(-) diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java b/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java index 50cdae3f3..2e8a24ed2 100644 --- a/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java +++ b/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java @@ -697,28 +697,35 @@ public class AsyncKuduClient implements AutoCloseable { } /** - * Delete a table with the specified name. The table is purged immediately. + * Delete a table with the specified name. * @param name the table's name + * @param reserveSeconds the soft deleted table to be alive time * @return a deferred object to track the progress of the deleteTable command */ - public Deferred<DeleteTableResponse> deleteTable(String name) { - return deleteTable(name, NO_SOFT_DELETED_STATE_RESERVED_SECONDS); + public Deferred<DeleteTableResponse> deleteTable(String name, + int reserveSeconds) { + checkIsClosed(); + DeleteTableRequest delete = new DeleteTableRequest(this.masterTable, + name, + timer, + defaultAdminOperationTimeoutMs, + reserveSeconds); + return sendRpcToTablet(delete); } /** * Delete a table with the specified name. + * The behavior of DeleteRPC is controlled by the + * '--default_deleted_table_reserve_seconds' flag on master. * @param name the table's name - * @param reserveSeconds the soft deleted table to be alive time * @return a deferred object to track the progress of the deleteTable command */ - public Deferred<DeleteTableResponse> deleteTable(String name, - int reserveSeconds) { + public Deferred<DeleteTableResponse> deleteTable(String name) { checkIsClosed(); DeleteTableRequest delete = new DeleteTableRequest(this.masterTable, name, timer, - defaultAdminOperationTimeoutMs, - reserveSeconds); + defaultAdminOperationTimeoutMs); return sendRpcToTablet(delete); } diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java b/java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java index 49b9ff461..5ecf13a70 100644 --- a/java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java +++ b/java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java @@ -34,7 +34,7 @@ class DeleteTableRequest extends KuduRpc<DeleteTableResponse> { private final String name; - private final int reserveSeconds; + private int reserveSeconds = -1; DeleteTableRequest(KuduTable table, String name, @@ -46,13 +46,23 @@ class DeleteTableRequest extends KuduRpc<DeleteTableResponse> { this.reserveSeconds = reserveSeconds; } + DeleteTableRequest(KuduTable table, + String name, + Timer timer, + long timeoutMillis) { + super(table, timer, timeoutMillis); + this.name = name; + } + @Override Message createRequestPB() { final Master.DeleteTableRequestPB.Builder builder = Master.DeleteTableRequestPB.newBuilder(); Master.TableIdentifierPB tableID = Master.TableIdentifierPB.newBuilder().setTableName(name).build(); builder.setTable(tableID); - builder.setReserveSeconds(reserveSeconds); + if (reserveSeconds >= 0) { + builder.setReserveSeconds(reserveSeconds); + } return builder.build(); } diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java b/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java index 382cce9e2..1536574df 100644 --- a/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java +++ b/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java @@ -163,12 +163,15 @@ public class KuduClient implements AutoCloseable { /** * Delete a table on the cluster with the specified name. + * The deleted table may turn to soft-deleted status with the flag + * default_deleted_table_reserve_seconds set to nonzero on the master side. + * * @param name the table's name * @return an rpc response object * @throws KuduException if anything went wrong */ public DeleteTableResponse deleteTable(String name) throws KuduException { - Deferred<DeleteTableResponse> d = asyncClient.deleteTable(name, 0); + Deferred<DeleteTableResponse> d = asyncClient.deleteTable(name); return joinAndHandleException(d); } diff --git a/src/kudu/client/client-internal.cc b/src/kudu/client/client-internal.cc index 63b5be4d3..ca6bd53d8 100644 --- a/src/kudu/client/client-internal.cc +++ b/src/kudu/client/client-internal.cc @@ -379,13 +379,15 @@ Status KuduClient::Data::DeleteTable(KuduClient* client, const string& table_name, const MonoTime& deadline, bool modify_external_catalogs, - uint32_t reserve_seconds) { + std::optional<uint32_t> reserve_seconds) { DeleteTableRequestPB req; DeleteTableResponsePB resp; req.mutable_table()->set_table_name(table_name); req.set_modify_external_catalogs(modify_external_catalogs); - req.set_reserve_seconds(reserve_seconds); + if (reserve_seconds) { + req.set_reserve_seconds(*reserve_seconds); + } Synchronizer sync; AsyncLeaderMasterRpc<DeleteTableRequestPB, DeleteTableResponsePB> rpc( deadline, client, BackoffType::EXPONENTIAL, req, &resp, diff --git a/src/kudu/client/client-internal.h b/src/kudu/client/client-internal.h index 7d70b5056..b29b77d91 100644 --- a/src/kudu/client/client-internal.h +++ b/src/kudu/client/client-internal.h @@ -20,6 +20,7 @@ #include <functional> #include <map> #include <memory> +#include <optional> #include <set> #include <string> #include <unordered_set> @@ -83,8 +84,6 @@ class RemoteTabletServer; class KuduClient::Data { public: - static const uint32_t kSoftDeletedTableReservationSeconds = 60 * 60 * 24 * 7; - Data(); ~Data(); @@ -124,7 +123,7 @@ class KuduClient::Data { const std::string& table_name, const MonoTime& deadline, bool modify_external_catalogs = true, - uint32_t reserve_seconds = kSoftDeletedTableReservationSeconds); + std::optional<uint32_t> reserve_seconds = std::nullopt); static Status RecallTable(KuduClient* client, const std::string& table_id, diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc index 071f88ccd..b7d5412ae 100644 --- a/src/kudu/client/client-test.cc +++ b/src/kudu/client/client-test.cc @@ -5102,8 +5102,8 @@ TEST_F(ClientTest, TestSoftDeleteAndReserveTable) { // Not allowed to delete the soft_deleted table with new reserve_seconds value. s = client_->SoftDeleteTable(kTableName, 600); ASSERT_TRUE(s.IsInvalidArgument()); - ASSERT_STR_CONTAINS(s.ToString(), Substitute("soft_deleted table $0 should not be deleted", - kTableName)); + ASSERT_STR_CONTAINS(s.ToString(), + Substitute("soft_deleted table $0 should not be soft deleted again", kTableName)); } { @@ -5355,8 +5355,14 @@ TEST_F(ClientTest, TestDeleteSoftDeleteStatusFromDeletedTableReserveSeconds) { ASSERT_OK(client_->ListTables(&tables)); ASSERT_TRUE(tables.empty()); + // The behavior of DeleteTable turn to soft-delete + // soft-deleted table should not be soft deleted + Status s = client_->DeleteTable(kTableName); + ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString(); + ASSERT_STR_CONTAINS(s.ToString(), + Substitute("soft_deleted table $0 should not be soft deleted again", kTableName)); // purge the soft-deleted table - ASSERT_OK(client_->DeleteTable(kTableName)); + ASSERT_OK(client_->SoftDeleteTable(kTableName, 0)); // No tables left. ASSERT_OK(client_->ListTables(&tables)); diff --git a/src/kudu/client/client.cc b/src/kudu/client/client.cc index 0706277ee..f47f0ef0d 100644 --- a/src/kudu/client/client.cc +++ b/src/kudu/client/client.cc @@ -510,7 +510,9 @@ Status KuduClient::IsCreateTableInProgress(const string& table_name, } Status KuduClient::DeleteTable(const string& table_name) { - return SoftDeleteTable(table_name); + // The default param 'reserve_seconds' not be set means the behavior of DeleteRPC is + // controlled by the 'default_deleted_table_reserve_seconds' flag. + return DeleteTableInCatalogs(table_name, true); } Status KuduClient::SoftDeleteTable(const string& table_name, @@ -520,10 +522,12 @@ Status KuduClient::SoftDeleteTable(const string& table_name, Status KuduClient::DeleteTableInCatalogs(const string& table_name, bool modify_external_catalogs, - uint32_t reserve_seconds) { + int32_t reserve_seconds) { MonoTime deadline = MonoTime::Now() + default_admin_operation_timeout(); + std::optional<uint32_t> reserve_time = reserve_seconds >= 0 ? + std::make_optional(reserve_seconds) : std::nullopt; return KuduClient::Data::DeleteTable(this, table_name, deadline, modify_external_catalogs, - reserve_seconds); + reserve_time); } Status KuduClient::RecallTable(const string& table_id, const string& new_table_name) { diff --git a/src/kudu/client/client.h b/src/kudu/client/client.h index cbe9877d0..834d77dce 100644 --- a/src/kudu/client/client.h +++ b/src/kudu/client/client.h @@ -56,9 +56,9 @@ namespace kudu { class AlterTableTest; class AuthzTokenTest; class ClientStressTest_TestUniqueClientIds_Test; -class MetaCacheLookupStressTest_PerfSynthetic_Test; class DisableWriteWhenExceedingQuotaTest; class KuduPartialRow; +class MetaCacheLookupStressTest_PerfSynthetic_Test; class MonoDelta; class Partition; class PartitionSchema; @@ -683,6 +683,8 @@ class KUDU_EXPORT KuduClient : public sp::enable_shared_from_this<KuduClient> { bool* create_in_progress); /// Delete/drop a table without reserving. + /// The deleted table may turn to soft-deleted status with the flag + /// --default_deleted_table_reserve_seconds set to nonzero on the master side. /// /// The delete operation or drop operation means that the service will directly /// delete the table after receiving the instruction. Which means that once we @@ -736,10 +738,12 @@ class KUDU_EXPORT KuduClient : public sp::enable_shared_from_this<KuduClient> { /// which the Kudu master has been configured to integrate with. /// @param [in] reserve_seconds /// Reserve seconds after being deleted. + /// Default value '-1' means not specified and the server will use master side config. + /// '0' means purge immediately and other values means reserve some seconds. /// @return Operation status. Status DeleteTableInCatalogs(const std::string& table_name, bool modify_external_catalogs, - uint32_t reserve_seconds = 0) KUDU_NO_EXPORT; + int32_t reserve_seconds = -1) KUDU_NO_EXPORT; /// Recall a deleted but still reserved table. /// diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index 22c56a3a7..916880a88 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -2410,18 +2410,36 @@ Status CatalogManager::SoftDeleteTableRpc(const DeleteTableRequestPB& req, bool is_soft_deleted_table = false; bool is_expired_table = false; Status s = GetTableStates(req.table(), kAllTableType, &is_soft_deleted_table, &is_expired_table); - if (s.ok() && is_soft_deleted_table && req.reserve_seconds() != 0) { + // The only error is NotFound, deal it after authorize by DeleteTableRpc(...) function. + if (s.IsNotFound()) { + return DeleteTableRpc(req, resp, rpc); + } + + DCHECK(s.ok()); + + // The 'reserve_seconds' is specified by the client, means the request coming from a newer + // Kudu client with the precise value for 'reserve_seconds', and the field's value from the + // request should be taken as-is regardless of the current setting of the + // '--default_deleted_table_reserve_seconds' flag at the server side. + // + // If the 'reserve_seconds' is not specified by the client, means the behavior of DeleteRPC is + // controlled by the '--default_deleted_table_reserve_seconds' flag. + bool enable_soft_delete_on_client = req.has_reserve_seconds() && req.reserve_seconds() != 0; + bool enable_soft_delete_on_master = FLAGS_default_deleted_table_reserve_seconds != 0; + bool delete_without_reserving = (req.has_reserve_seconds() && req.reserve_seconds() == 0) || + (!req.has_reserve_seconds() && !enable_soft_delete_on_master); + // Soft-delete a soft-deleted table is not allowed. + if (is_soft_deleted_table && + // soft-delete has enabled + (enable_soft_delete_on_client || enable_soft_delete_on_master) && + !delete_without_reserving) { return SetupError( - Status::InvalidArgument(Substitute("soft_deleted table $0 should not be deleted", + Status::InvalidArgument(Substitute("soft_deleted table $0 should not be soft deleted again", req.table().table_name())), resp, MasterErrorPB::TABLE_SOFT_DELETED); } - // Reserve seconds equal 0 means delete it directly if default_deleted_table_reserve_seconds - // set to 0, which means the cluster-wide behavior of the DeleteTable() RPC is keep default, - // or the table is in soft-deleted status. - if (req.reserve_seconds() == 0 && - (FLAGS_default_deleted_table_reserve_seconds == 0 || is_soft_deleted_table)) { + if (delete_without_reserving) { return DeleteTableRpc(req, resp, rpc); } diff --git a/src/kudu/master/master.proto b/src/kudu/master/master.proto index d4e9699ea..d527ec11c 100644 --- a/src/kudu/master/master.proto +++ b/src/kudu/master/master.proto @@ -586,6 +586,11 @@ message DeleteTableRequestPB { optional bool modify_external_catalogs = 2 [default = true]; // Reserve seconds after the table has been deleted. + // If this field is specified by the client, means the request coming from a newer Kudu + // client with the precise value for 'reserve_seconds', and the field's value from the request + // should be taken as-is regardless of the current setting of the '--default_deleted_table_reserve_seconds' + // flag at the server side. + // Otherwise, the behavior of DeleteRPC is controlled by the '--default_deleted_table_reserve_seconds' flag. optional uint32 reserve_seconds = 3; } diff --git a/src/kudu/tools/kudu-admin-test.cc b/src/kudu/tools/kudu-admin-test.cc index fd8750ae5..0a11686d6 100644 --- a/src/kudu/tools/kudu-admin-test.cc +++ b/src/kudu/tools/kudu-admin-test.cc @@ -1652,8 +1652,7 @@ TEST_F(AdminCliTest, TestDeleteTable) { "table", "delete", master_address, - kTableId, - "-reserve_seconds=0" + kTableId ); vector<string> tables; diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc index 76b350707..d08859397 100644 --- a/src/kudu/tools/kudu-tool-test.cc +++ b/src/kudu/tools/kudu-tool-test.cc @@ -4576,7 +4576,7 @@ TEST_F(ToolTest, TestDeleteTable) { // Delete the table. NO_FATALS(RunActionStdoutNone(Substitute( - "table delete $0 $1 --nomodify_external_catalogs -reserve_seconds=0", + "table delete $0 $1 --nomodify_external_catalogs", master_addr, kTableName))); // Check that the table does not exist. @@ -4633,7 +4633,8 @@ TEST_F(ToolTest, TestRecallTable) { // Soft-delete the table. string out; NO_FATALS(RunActionStdoutNone(Substitute( - "table delete $0 $1 --reserve_seconds=300", master_addr, kTableName))); + "table delete $0 $1 --reserve_seconds=300", + master_addr, kTableName))); // List soft_deleted table. vector<string> kudu_tables; diff --git a/src/kudu/tools/tool_action_table.cc b/src/kudu/tools/tool_action_table.cc index 56608c9f5..fa554d4c8 100644 --- a/src/kudu/tools/tool_action_table.cc +++ b/src/kudu/tools/tool_action_table.cc @@ -93,6 +93,7 @@ using std::endl; using std::find_if; using std::make_unique; using std::map; +using std::nullopt; using std::ostringstream; using std::pair; using std::set; @@ -198,9 +199,9 @@ DEFINE_bool(show_avro_format_schema, false, "Display the table schema in avro format. When enabled it only outputs the " "table schema in Avro format without any other information like " "partition/owner/comments. It cannot be used in conjunction with other flags"); -DEFINE_uint32(reserve_seconds, 0, +DEFINE_int32(reserve_seconds, -1, "Grace period before purging a soft-deleted table, in seconds. " - "If set to 0, table is purged once it dropped/deleted."); + "If set to 0, it would be purged when a table is dropped/deleted."); DECLARE_bool(create_table); DECLARE_bool(fault_tolerant); @@ -579,7 +580,7 @@ Status DescribeTable(const RunnerContext& context) { const KuduSchema& schema = table->schema(); if (FLAGS_show_avro_format_schema) { return PopulateAvroSchema(FindOrDie(context.required_args, kTableNameArg), - client->cluster_id(), schema); + client->cluster_id(), schema); } cout << "TABLE " << table_name << " " << schema.ToString() << endl; // The partition schema with current range partitions.
