This is an automated email from the ASF dual-hosted git repository.
laiyingchun pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new b46bbddb4 KUDU-3326 disable compaction for soft-deleted table
b46bbddb4 is described below
commit b46bbddb42714706725254aa87e21b354611eeb3
Author: kedeng <[email protected]>
AuthorDate: Mon Jul 10 19:19:23 2023 +0800
KUDU-3326 disable compaction for soft-deleted table
The tables in the 'SOFT_DELETED' state will no longer
process insert and update requests, so the compaction
and other related operations will become redundant and
also waste resources.
In this patch, we added a post-operation to disable
compaction for the soft-deleted operation and a
post-operation to enable compaction for the recall
process.
To verify the correctness of the new logic, we also
added new unit tests.
Change-Id: I060810051613a19e6f5c6506effda2d698528839
Reviewed-on: http://gerrit.cloudera.org:8080/20178
Tested-by: Kudu Jenkins
Reviewed-by: Yingchun Lai <[email protected]>
---
src/kudu/client/client-test.cc | 62 ++++++++++++++++++++++
src/kudu/master/catalog_manager.cc | 104 +++++++++++++++++++++++++++++++------
src/kudu/master/catalog_manager.h | 33 +++++++++++-
src/kudu/master/master-test.cc | 93 +++++++++++++++++++++------------
4 files changed, 242 insertions(+), 50 deletions(-)
diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc
index beb6d220b..600013465 100644
--- a/src/kudu/client/client-test.cc
+++ b/src/kudu/client/client-test.cc
@@ -5351,6 +5351,8 @@ TEST_F(ClientTest,
TestSoftDeleteAndRecallAfterReserveTimeTable) {
vector<string> tables;
ASSERT_OK(client_->ListSoftDeletedTables(&tables));
ASSERT_EQ(0, tables.size());
+ ASSERT_OK(client_->ListTables(&tables));
+ ASSERT_EQ(0, tables.size());
// Try to recall the table.
Status s = client_->RecallTable(client_table_->id());
@@ -5360,6 +5362,66 @@ TEST_F(ClientTest,
TestSoftDeleteAndRecallAfterReserveTimeTable) {
ASSERT_TRUE(tables.empty());
}
+TEST_F(ClientTest, TestCompactionOfSoftDeletedAndRecalledTable) {
+ // Open the table before deleting it.
+ ASSERT_OK(client_->OpenTable(kTableName, &client_table_));
+ const string table_id = client_table_->id();
+ // Check the compaction is enabled.
+ map<string, string> extra_configs = client_table_->extra_configs();
+ ASSERT_FALSE(ContainsKey(extra_configs, "kudu.table.disable_compaction"));
+
+ {
+ // Soft delete the table.
+ ASSERT_OK(client_->SoftDeleteTable(kTableName, 60000));
+
+ // Do some soft-deleted table checks.
+ vector<string> tables;
+ ASSERT_OK(client_->ListTables(&tables));
+ ASSERT_EQ(0, tables.size());
+ ASSERT_OK(client_->ListSoftDeletedTables(&tables));
+ ASSERT_EQ(1, tables.size());
+ ASSERT_EQ(string(kTableName), tables[0]);
+
+ // Wait for the table has 'disable_compaction' config.
+ AssertEventually([&] {
+ // Reopen table.
+ shared_ptr<KuduTable> t;
+ ASSERT_OK(client_->OpenTable(kTableName, &t));
+ // The compaction is disabled for soft_deleted table.
+ map<string, string> extra_configs = t->extra_configs();
+ ASSERT_TRUE(ContainsKey(extra_configs, "kudu.table.disable_compaction"));
+ ASSERT_EQ("1", extra_configs["kudu.table.disable_compaction"]);
+ }, MonoDelta::FromSeconds(5));
+ NO_PENDING_FATALS();
+ }
+
+ {
+ // Recall and reopen table.
+ vector<string> tables;
+ ASSERT_OK(client_->ListSoftDeletedTables(&tables));
+ ASSERT_EQ(1, tables.size());
+
+ ASSERT_OK(client_->RecallTable(table_id));
+ ASSERT_OK(client_->ListTables(&tables));
+ ASSERT_EQ(1, tables.size());
+ ASSERT_EQ(kTableName, tables[0]);
+ ASSERT_OK(client_->ListSoftDeletedTables(&tables));
+ ASSERT_TRUE(tables.empty());
+
+ // The compaction is enabled for recalled table.
+ AssertEventually([&] {
+ // Reopen table.
+ shared_ptr<KuduTable> t;
+ ASSERT_OK(client_->OpenTable(kTableName, &t));
+ // The compaction is enabled for non soft deleted table.
+ map<string, string> extra_configs = t->extra_configs();
+ ASSERT_TRUE(ContainsKey(extra_configs, "kudu.table.disable_compaction"));
+ ASSERT_EQ("0", extra_configs["kudu.table.disable_compaction"]);
+ }, MonoDelta::FromSeconds(5));
+ NO_PENDING_FATALS();
+ }
+}
+
TEST_F(ClientTest, TestDeleteWithDeletedTableReserveSeconds) {
// Open the table before deleting it.
ASSERT_OK(client_->OpenTable(kTableName, &client_table_));
diff --git a/src/kudu/master/catalog_manager.cc
b/src/kudu/master/catalog_manager.cc
index b2bfac5a5..53007c7ef 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -2635,10 +2635,60 @@ Status CatalogManager::SoftDeleteTable(const
DeleteTableRequestPB& req,
TRACE("Committing in-memory state");
l.Commit();
+ // 7. Disable compaction for soft-deleted table.
+ TRACE("Disable compaction for soft-deleted table");
+ RETURN_NOT_OK(DisableCompactionForSoftDeletedTable(req, rpc));
+
VLOG(1) << "Soft deleted table " << table->ToString();
return Status::OK();
}
+Status CatalogManager::DisableCompactionForSoftDeletedTable(const
DeleteTableRequestPB& req,
+ rpc::RpcContext*
rpc) {
+ AlterTableRequestPB alter_req;
+ alter_req.mutable_table()->CopyFrom(req.table());
+ (*alter_req.mutable_new_extra_configs())[kTableDisableCompaction] = "true";
+ AlterTableResponsePB alter_resp;
+ // Soft deleted tables cannot set extra configs so we should do this with
+ // 'alter_type' set to kForceToSoftDeleted.
+ Status s = AlterTableRpc(alter_req, &alter_resp, rpc,
+ /* alter_type */ AlterType::kForceToSoftDeleted);
+ if (!s.ok()) {
+ s = s.CloneAndPrepend("an error occurred while disable compaction for soft
deleted table.");
+ LOG(WARNING) << s.ToString();
+ return s;
+ }
+
+ // After the extra config set, we need change the table state to
'soft-deleted'.
+ // But AlterTableRpc is an asynchronous operation, and we cannot wait here
for the operation
+ // to complete, as it will cause the soft deletion to timeout. Ultimately,
it was decided to
+ // restore the table's state to the 'SOFT_DELETED' state in
HandleTabletSchemaVersionReport.
+
+ return Status::OK();
+}
+
+Status CatalogManager::EnableCompactionforRecalledTable(const
RecallDeletedTableRequestPB& req,
+ rpc::RpcContext* rpc) {
+ AlterTableRequestPB alter_req;
+ alter_req.mutable_table()->CopyFrom(req.table());
+ (*alter_req.mutable_new_extra_configs())[kTableDisableCompaction] = "false";
+ AlterTableResponsePB alter_resp;
+ // Soft deleted tables cannot set extra configs so we should do this with
+ // 'alter_type' set to kForceToSoftDeleted.
+ Status s = AlterTableRpc(alter_req, &alter_resp, rpc,
+ /* alter_type */ AlterType::kForceToSoftDeleted);
+ if (!s.ok()) {
+ s = s.CloneAndPrepend("an error occurred while enable compaction for soft
deleted table.");
+ LOG(WARNING) << s.ToString();
+ return s;
+ }
+
+ // The state of the recalled soft-deleted table is normal 'RUNNING' state,
so there is no need
+ // to process the state of the table here.
+
+ return Status::OK();
+}
+
Status CatalogManager::DeleteTableRpc(const DeleteTableRequestPB& req,
DeleteTableResponsePB* resp,
rpc::RpcContext* rpc) {
@@ -2838,7 +2888,8 @@ Status CatalogManager::RecallDeletedTableRpc(const
RecallDeletedTableRequestPB&
alter_req.set_new_table_name(req.new_table_name());
AlterTableResponsePB alter_resp;
- Status s = AlterTableRpc(alter_req, &alter_resp, rpc);
+ Status s = AlterTableRpc(alter_req, &alter_resp, rpc,
+ /* alter_type */ AlterType::kForceToSoftDeleted);
if (!s.ok()) {
s = s.CloneAndPrepend("an error occurred while renaming the recalled
table.");
LOG(WARNING) << s.ToString();
@@ -2846,6 +2897,10 @@ Status CatalogManager::RecallDeletedTableRpc(const
RecallDeletedTableRequestPB&
}
}
+ // Enable compaction for recall deleted table.
+ TRACE("Enable compaction for recall deleted table");
+ RETURN_NOT_OK(EnableCompactionforRecalledTable(req, rpc));
+
return Status::OK();
}
@@ -3323,19 +3378,24 @@ Status CatalogManager::ApplyAlterPartitioningSteps(
Status CatalogManager::AlterTableRpc(const AlterTableRequestPB& req,
AlterTableResponsePB* resp,
- rpc::RpcContext* rpc) {
+ rpc::RpcContext* rpc,
+ AlterType alter_type) {
LOG(INFO) << Substitute("Servicing AlterTable request from $0:\n$1",
RequestorString(rpc), SecureShortDebugString(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);
- // Alter soft_deleted table is not allowed.
- if (s.ok() && is_soft_deleted_table) {
- return SetupError(
- Status::InvalidArgument(Substitute("soft_deleted table $0 should not
be altered",
- req.table().table_name())),
- resp, MasterErrorPB::TABLE_SOFT_DELETED);
+ Status s;
+ if (alter_type == AlterType::kNormal) {
+ bool is_soft_deleted_table = false;
+ bool is_expired_table = false;
+ s = GetTableStates(req.table(), kAllTableType, &is_soft_deleted_table,
&is_expired_table);
+ // Alter soft_deleted table is not allowed, unless we have a reason to
force a alter,
+ // such as turning off compaction for a soft-deleted table.
+ if (s.ok() && is_soft_deleted_table) {
+ return SetupError(
+ Status::InvalidArgument(Substitute("soft_deleted table $0 should not
be altered",
+ req.table().table_name())),
+ resp, MasterErrorPB::TABLE_SOFT_DELETED);
+ }
}
leader_lock_.AssertAcquiredForReading();
@@ -5976,11 +6036,25 @@ void CatalogManager::HandleTabletSchemaVersionReport(
return;
}
- // Update the state from altering to running and remove the last fully
- // applied schema (if it exists).
+ // Update the state from altering and remove the last fully applied
+ // schema (if it exists).
+ // The final state depends on the state before the alteration. If the
+ // alteration is for a soft-deleted table, the state will change to
+ // 'SOFT_DELETED'. If the alteration is for a normal table, the state
+ // will change to 'RUNNING'.
l.mutable_data()->pb.clear_fully_applied_schema();
- l.mutable_data()->set_state(SysTablesEntryPB::RUNNING,
- Substitute("Current schema version=$0",
current_version));
+ // 'soft_deleted_reserved_seconds' is only exist for 'SOFT_DELETED' state.
+ if (l.mutable_data()->pb.has_soft_deleted_reserved_seconds() &&
+ l.mutable_data()->pb.soft_deleted_reserved_seconds() != UINT32_MAX) {
+ string deletion_msg = "Table soft deleted at " +
+ l.mutable_data()->pb.has_delete_timestamp() ?
+
std::to_string(l.mutable_data()->pb.delete_timestamp()) :
+ LocalTimeAsString();
+ l.mutable_data()->set_state(SysTablesEntryPB::SOFT_DELETED, deletion_msg);
+ } else {
+ l.mutable_data()->set_state(SysTablesEntryPB::RUNNING,
+ Substitute("Current schema version=$0",
current_version));
+ }
{
SysCatalogTable::Actions actions;
diff --git a/src/kudu/master/catalog_manager.h
b/src/kudu/master/catalog_manager.h
index 159a7a243..5675c46bd 100644
--- a/src/kudu/master/catalog_manager.h
+++ b/src/kudu/master/catalog_manager.h
@@ -261,8 +261,18 @@ struct PersistentTableInfo {
pb.state() == SysTablesEntryPB::SOFT_DELETED;
}
+ // To improve the efficiency of the GC scheduler during the soft
+ // deletion process, we need to disable the corresponding table's
+ // compaction through 'AlterTable'. This is why we introduced a new
+ // approach to determine the soft deletion status based on the table's
+ // status and reserved time, as the existing approach using state to
+ // determine the soft deletion status may return an incorrect result
+ // during the Alter table process.
bool is_soft_deleted() const {
- return pb.state() == SysTablesEntryPB::SOFT_DELETED;
+ return pb.state() == SysTablesEntryPB::SOFT_DELETED ||
+ (pb.state() != SysTablesEntryPB::REMOVED &&
+ pb.has_soft_deleted_reserved_seconds() &&
+ pb.soft_deleted_reserved_seconds() != UINT32_MAX);
}
// Expired table must in SOFT_DELETED state.
@@ -691,13 +701,23 @@ class CatalogManager : public
tserver::TabletReplicaLookupIf {
RecallDeletedTableResponsePB* resp,
rpc::RpcContext* rpc) WARN_UNUSED_RESULT;
+ enum class AlterType {
+ // Only normal type tables are allowed to be altered, not soft-deleted
tables.
+ kNormal = 0,
+ // Alterations are allowed for both normal type tables and soft-deleted
tables.
+ kForceToSoftDeleted
+ };
+
// Alter the specified table in response to an AlterTableRequest RPC.
//
// The RPC context is provided for logging/tracing purposes,
// but this function does not itself respond to the RPC.
+ // The 'alter_type' used by soft-delete function, which means the table
+ // state will not change to 'ALTERING' for disable the compaction.
Status AlterTableRpc(const AlterTableRequestPB& req,
AlterTableResponsePB* resp,
- rpc::RpcContext* rpc);
+ rpc::RpcContext* rpc,
+ AlterType alter_type = AlterType::kNormal);
// Alter the specified table in response to an 'ALTER TABLE' HMS
// notification log listener event.
@@ -986,6 +1006,15 @@ class CatalogManager : public
tserver::TabletReplicaLookupIf {
RecallDeletedTableResponsePB* resp,
rpc::RpcContext* rpc);
+ // Disable compaction for soft deleted table to improve the scheduling
performance
+ // of the compaction.
+ Status DisableCompactionForSoftDeletedTable(const DeleteTableRequestPB& req,
+ rpc::RpcContext* rpc);
+
+ // Enable compaction for recalled table to make the recalled table turn to
normal.
+ Status EnableCompactionforRecalledTable(const RecallDeletedTableRequestPB&
req,
+ rpc::RpcContext* rpc);
+
// Check whether the table's write limit is reached,
// if true, the write permission should be disabled.
static bool IsTableWriteDisabled(const scoped_refptr<TableInfo>& table,
diff --git a/src/kudu/master/master-test.cc b/src/kudu/master/master-test.cc
index 4652f1817..85c111e01 100644
--- a/src/kudu/master/master-test.cc
+++ b/src/kudu/master/master-test.cc
@@ -3644,27 +3644,41 @@ TEST_F(MasterStartupTest, StartupWebPage) {
}
TEST_F(MasterTest, GetTableStatesWithName) {
- const char* kTableName = "testtable";
+ const char* kTableNameA = "testtablea";
+ const char* kTableNameB = "testtableb";
const Schema kTableSchema({ ColumnSchema("key", INT32) }, 1);
bool is_soft_deleted_table = false;
bool is_expired_table = false;
- TableIdentifierPB table_identifier;
- table_identifier.set_table_name(kTableName);
-
- // Create a new table.
- ASSERT_OK(CreateTable(kTableName, kTableSchema));
+ TableIdentifierPB table_identifier_a;
+ table_identifier_a.set_table_name(kTableNameA);
+ TableIdentifierPB table_identifier_b;
+ table_identifier_b.set_table_name(kTableNameB);
+
+ // Create new tables.
+ ASSERT_OK(CreateTable(kTableNameA, kTableSchema));
+ ASSERT_OK(CreateTable(kTableNameB, kTableSchema));
ListTablesResponsePB tables;
NO_FATALS(DoListAllTables(&tables));
- ASSERT_EQ(1, tables.tables_size());
- ASSERT_EQ(kTableName, tables.tables(0).name());
- string table_id = tables.tables(0).id();
- ASSERT_FALSE(table_id.empty());
+ ASSERT_EQ(2, tables.tables_size());
+ string table_id_a;
+ string table_id_b;
+ for (const auto& table : tables.tables()) {
+ if (table.name() == kTableNameA) {
+ table_id_a = table.id();
+ } else if (table.name() == kTableNameB) {
+ table_id_b = table.id();
+ } else {
+ ASSERT_FALSE(1);
+ }
+ }
+ ASSERT_FALSE(table_id_a.empty());
+ ASSERT_FALSE(table_id_b.empty());
{
// Default table is not expired.
CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
ASSERT_OK(master_->catalog_manager()->GetTableStates(
- table_identifier, CatalogManager::TableInfoMapType::kAllTableType,
+ table_identifier_a, CatalogManager::TableInfoMapType::kAllTableType,
&is_soft_deleted_table, &is_expired_table));
ASSERT_FALSE(is_soft_deleted_table);
ASSERT_FALSE(is_expired_table);
@@ -3673,22 +3687,21 @@ TEST_F(MasterTest, GetTableStatesWithName) {
{
// In reserve time, table is not expired.
CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
- ASSERT_OK(SoftDelete(kTableName, 100));
+ ASSERT_OK(SoftDelete(kTableNameA, 100));
ASSERT_OK(master_->catalog_manager()->GetTableStates(
- table_identifier, CatalogManager::TableInfoMapType::kAllTableType,
+ table_identifier_a, CatalogManager::TableInfoMapType::kAllTableType,
&is_soft_deleted_table, &is_expired_table));
ASSERT_TRUE(is_soft_deleted_table);
ASSERT_FALSE(is_expired_table);
- ASSERT_OK(RecallTable(table_id));
}
{
// After reserve time, table is expired.
CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
- ASSERT_OK(SoftDelete(kTableName, 1));
+ ASSERT_OK(SoftDelete(kTableNameB, 1));
SleepFor(MonoDelta::FromSeconds(1));
ASSERT_OK(master_->catalog_manager()->GetTableStates(
- table_identifier, CatalogManager::TableInfoMapType::kAllTableType,
+ table_identifier_b, CatalogManager::TableInfoMapType::kAllTableType,
&is_soft_deleted_table, &is_expired_table));
ASSERT_TRUE(is_soft_deleted_table);
ASSERT_TRUE(is_expired_table);
@@ -3696,51 +3709,65 @@ TEST_F(MasterTest, GetTableStatesWithName) {
}
TEST_F(MasterTest, GetTableStatesWithId) {
- const char* kTableName = "testtable";
+ const char* kTableNameA = "testtablea";
+ const char* kTableNameB = "testtableb";
const Schema kTableSchema({ ColumnSchema("key", INT32) }, 1);
bool is_soft_deleted_table = false;
bool is_expired_table = false;
- // Create a new table.
- ASSERT_OK(CreateTable(kTableName, kTableSchema));
+ // Create new tables.
+ ASSERT_OK(CreateTable(kTableNameA, kTableSchema));
+ ASSERT_OK(CreateTable(kTableNameB, kTableSchema));
ListTablesResponsePB tables;
NO_FATALS(DoListAllTables(&tables));
- ASSERT_EQ(1, tables.tables_size());
- ASSERT_EQ(kTableName, tables.tables(0).name());
- string table_id = tables.tables(0).id();
- ASSERT_FALSE(table_id.empty());
- TableIdentifierPB table_identifier;
- table_identifier.set_table_id(table_id);
+ ASSERT_EQ(2, tables.tables_size());
+ string table_id_a;
+ string table_id_b;
+ for (const auto& table : tables.tables()) {
+ if (table.name() == kTableNameA) {
+ table_id_a = table.id();
+ } else if (table.name() == kTableNameB) {
+ table_id_b = table.id();
+ } else {
+ ASSERT_FALSE(1);
+ }
+ }
+ ASSERT_FALSE(table_id_a.empty());
+ ASSERT_FALSE(table_id_b.empty());
+
+ TableIdentifierPB table_identifier_a;
+ table_identifier_a.set_table_id(table_id_a);
+ TableIdentifierPB table_identifier_b;
+ table_identifier_b.set_table_id(table_id_b);
{
// Default table is not expired.
CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
ASSERT_OK(master_->catalog_manager()->GetTableStates(
- table_identifier, CatalogManager::TableInfoMapType::kAllTableType,
+ table_identifier_a, CatalogManager::TableInfoMapType::kAllTableType,
&is_soft_deleted_table, &is_expired_table));
ASSERT_FALSE(is_soft_deleted_table);
ASSERT_FALSE(is_expired_table);
}
{
- // In reserve time, table is not expired.
+ // In reserve time, kTableNameA is not expired.
CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
- ASSERT_OK(SoftDelete(kTableName, 100));
+ ASSERT_OK(SoftDelete(kTableNameA, 100));
ASSERT_OK(master_->catalog_manager()->GetTableStates(
- table_identifier, CatalogManager::TableInfoMapType::kAllTableType,
+ table_identifier_a, CatalogManager::TableInfoMapType::kAllTableType,
&is_soft_deleted_table, &is_expired_table));
ASSERT_TRUE(is_soft_deleted_table);
ASSERT_FALSE(is_expired_table);
- ASSERT_OK(RecallTable(table_id));
}
{
- // After reserve time, table is expired.
+ // After reserve time, kTableNameB is expired.
CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
- ASSERT_OK(SoftDelete(kTableName, 1));
+ ASSERT_OK(SoftDelete(kTableNameB, 1));
SleepFor(MonoDelta::FromSeconds(1));
ASSERT_OK(master_->catalog_manager()->GetTableStates(
- table_identifier, CatalogManager::TableInfoMapType::kAllTableType,
+ table_identifier_b, CatalogManager::TableInfoMapType::kAllTableType,
&is_soft_deleted_table, &is_expired_table));
ASSERT_TRUE(is_soft_deleted_table);
ASSERT_TRUE(is_expired_table);
