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 <acelyc1112009@gmail.com>
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 50cdae3..2e8a24e 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,15 +697,6 @@
}
/**
- * Delete a table with the specified name. The table is purged immediately.
- * @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, NO_SOFT_DELETED_STATE_RESERVED_SECONDS);
- }
-
- /**
* Delete a table with the specified name.
* @param name the table's name
* @param reserveSeconds the soft deleted table to be alive time
@@ -723,6 +714,22 @@
}
/**
+ * 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
+ * @return a deferred object to track the progress of the deleteTable command
+ */
+ public Deferred<DeleteTableResponse> deleteTable(String name) {
+ checkIsClosed();
+ DeleteTableRequest delete = new DeleteTableRequest(this.masterTable,
+ name,
+ timer,
+ defaultAdminOperationTimeoutMs);
+ return sendRpcToTablet(delete);
+ }
+
+ /**
* Recall a soft-deleted table on the cluster with the specified id
* @param id the table's id
* @return a deferred object to track the progress of the recall command
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 49b9ff4..5ecf13a 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 @@
private final String name;
- private final int reserveSeconds;
+ private int reserveSeconds = -1;
DeleteTableRequest(KuduTable table,
String name,
@@ -46,13 +46,23 @@
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 382cce9..1536574 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 @@
/**
* 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 63b5be4..ca6bd53 100644
--- a/src/kudu/client/client-internal.cc
+++ b/src/kudu/client/client-internal.cc
@@ -379,13 +379,15 @@
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 7d70b50..b29b77d 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 KuduClient::Data {
public:
- static const uint32_t kSoftDeletedTableReservationSeconds = 60 * 60 * 24 * 7;
-
Data();
~Data();
@@ -124,7 +123,7 @@
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 071f88c..b7d5412 100644
--- a/src/kudu/client/client-test.cc
+++ b/src/kudu/client/client-test.cc
@@ -5102,8 +5102,8 @@
// 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 @@
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 0706277..f47f0ef 100644
--- a/src/kudu/client/client.cc
+++ b/src/kudu/client/client.cc
@@ -510,7 +510,9 @@
}
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::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 cbe9877..834d77d 100644
--- a/src/kudu/client/client.h
+++ b/src/kudu/client/client.h
@@ -56,9 +56,9 @@
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 @@
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 @@
/// 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 22c56a3..916880a 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -2410,18 +2410,36 @@
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 d4e9699..d527ec1 100644
--- a/src/kudu/master/master.proto
+++ b/src/kudu/master/master.proto
@@ -586,6 +586,11 @@
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 fd8750a..0a11686 100644
--- a/src/kudu/tools/kudu-admin-test.cc
+++ b/src/kudu/tools/kudu-admin-test.cc
@@ -1652,8 +1652,7 @@
"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 76b3507..d088593 100644
--- a/src/kudu/tools/kudu-tool-test.cc
+++ b/src/kudu/tools/kudu-tool-test.cc
@@ -4576,7 +4576,7 @@
// 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 @@
// 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 56608c9..fa554d4 100644
--- a/src/kudu/tools/tool_action_table.cc
+++ b/src/kudu/tools/tool_action_table.cc
@@ -93,6 +93,7 @@
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 @@
"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 @@
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.
