Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KVStore: Fix unreleased Region instance #9763

Merged
merged 7 commits into from
Jan 7, 2025
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
format
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
  • Loading branch information
CalvinNeo committed Jan 3, 2025
commit 85e2d9785cdcde5e8f2c8a923d45816d3f66b0d1
5 changes: 5 additions & 0 deletions dbms/src/Common/TiFlashMetrics.h
Original file line number Diff line number Diff line change
@@ -576,6 +576,11 @@ static_assert(RAFT_REGION_BIG_WRITE_THRES * 4 < RAFT_REGION_BIG_WRITE_MAX, "Inva
F(type_tikv_server_issue, {{"type", "tikv_server_issue"}}), \
F(type_tikv_lock, {{"type", "tikv_lock"}}), \
F(type_other, {{"type", "other"}})) \
M(tiflash_raft_classes_count, \
"Raft classes counter", \
Gauge, \
F(type_region, {{"type", "region"}}), \
F(type_decoded_lockcf, {{"type", "decoded_lockcf"}})) \
/* required by DBaaS */ \
M(tiflash_server_info, \
"Indicate the tiflash server info, and the value is the start timestamp (s).", \
3 changes: 1 addition & 2 deletions dbms/src/Storages/KVStore/KVStore.cpp
Original file line number Diff line number Diff line change
@@ -57,7 +57,6 @@ KVStore::KVStore(Context & context)
: region_persister(
context.getSharedContextDisagg()->isDisaggregatedComputeMode() ? nullptr
: std::make_unique<RegionPersister>(context))
, raft_cmd_res(std::make_unique<RaftCommandResult>())
, log(Logger::get())
, region_compact_log_min_rows(40 * 1024)
, region_compact_log_min_bytes(32 * 1024 * 1024)
@@ -363,7 +362,7 @@ void KVStore::handleDestroy(UInt64 region_id, TMTContext & tmt, const KVStoreTas
LOG_INFO(log, "region_id={} not found, might be removed already", region_id);
return;
}
LOG_INFO(log, "Handle destroy {}", region->toString());
LOG_INFO(log, "Handle destroy {}, refCount {}", region->toString(), region.use_count());
region->setPendingRemove();
removeRegion(
region_id,
3 changes: 0 additions & 3 deletions dbms/src/Storages/KVStore/KVStore.h
Original file line number Diff line number Diff line change
@@ -379,9 +379,6 @@ class KVStore final : private boost::noncopyable

mutable std::mutex task_mutex;

// raft_cmd_res stores the result of applying raft cmd. It must be protected by task_mutex.
std::unique_ptr<RaftCommandResult> raft_cmd_res;

LoggerPtr log;

std::atomic<UInt64> region_compact_log_min_rows;
4 changes: 2 additions & 2 deletions dbms/src/Storages/KVStore/MultiRaft/RaftCommandsKVS.cpp
Original file line number Diff line number Diff line change
@@ -216,9 +216,9 @@ EngineStoreApplyRes KVStore::handleAdminRaftCmd(
curr_region.orphanKeysInfo().advanceAppliedIndex(index);
}

RaftCommandResult result;
curr_region.makeRaftCommandDelegate(task_lock)
.handleAdminRaftCmd(request, response, index, term, *this, region_table, *raft_cmd_res);
RaftCommandResult & result = *raft_cmd_res;
.handleAdminRaftCmd(request, response, index, term, *this, region_table, result);

// After region split / merge, try to flush it
const auto try_to_flush_region = [&tmt](const RegionPtr & region) {
10 changes: 9 additions & 1 deletion dbms/src/Storages/KVStore/MultiRaft/RegionData.cpp
Original file line number Diff line number Diff line change
@@ -310,7 +310,8 @@ void RegionData::deserialize(ReadBuffer & buf, RegionData & region_data)
total_size += RegionWriteCFData::deserialize(buf, region_data.write_cf);
total_size += RegionLockCFData::deserialize(buf, region_data.lock_cf);

region_data.cf_data_size += total_size;
region_data.cf_data_size = total_size;
reportAlloc(total_size);
}

RegionWriteCFData & RegionData::writeCF()
@@ -348,6 +349,13 @@ RegionData::RegionData(RegionData && data)
, cf_data_size(data.cf_data_size.load())
{}


RegionData::~RegionData()
{
reportDealloc(cf_data_size);
cf_data_size = 0;
}

RegionData & RegionData::operator=(RegionData && rhs)
{
write_cf = std::move(rhs.write_cf);
1 change: 1 addition & 0 deletions dbms/src/Storages/KVStore/MultiRaft/RegionData.h
Original file line number Diff line number Diff line change
@@ -81,6 +81,7 @@ class RegionData
const RegionLockCFData & lockCF() const;

RegionData() = default;
~RegionData();

RegionData(RegionData && data);
RegionData & operator=(RegionData &&);
3 changes: 1 addition & 2 deletions dbms/src/Storages/KVStore/MultiRaft/RegionSerde.cpp
Original file line number Diff line number Diff line change
@@ -158,12 +158,11 @@ RegionPtr Region::deserializeImpl(

// deserialize data
RegionData::deserialize(buf, region->data);
region->data.reportAlloc(region->data.cf_data_size);

// restore other var according to meta
region->last_restart_log_applied = region->appliedIndex();
region->setLastCompactLogApplied(region->appliedIndex());
return region;
}

} // namespace DB
} // namespace DB
6 changes: 4 additions & 2 deletions dbms/src/Storages/KVStore/Region.cpp
Original file line number Diff line number Diff line change
@@ -340,11 +340,13 @@ Region::Region(DB::RegionMeta && meta_, const TiFlashRaftProxyHelper * proxy_hel
, keyspace_id(meta.getRange()->getKeyspaceID())
, mapped_table_id(meta.getRange()->getMappedTableID())
, proxy_helper(proxy_helper_)
{}
{
GET_METRIC(tiflash_raft_classes_count, type_region).Increment(1);
}

Region::~Region()
{
data.reportDealloc(data.cf_data_size);
GET_METRIC(tiflash_raft_classes_count, type_region).Decrement();
}

TableID Region::getMappedTableID() const
5 changes: 4 additions & 1 deletion dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp
Original file line number Diff line number Diff line change
@@ -123,8 +123,11 @@ try
RegionPtr region = tests::makeRegion(702, start, end, proxy_helper.get());
region->insert("default", TiKVKey::copyFrom(str_key), TiKVValue::copyFrom(str_val_default));
ASSERT_EQ(root_of_kvstore_mem_trackers->get(), str_key.dataSize() + str_val_default.size());
// 702 is not registed, so we persist as 1.
tryPersistRegion(kvs, 1);
reloadKVSFromDisk();
root_of_kvstore_mem_trackers->reset();
ASSERT_EQ(root_of_kvstore_mem_trackers->get(), 0);
reloadKVSFromDisk(false);
ASSERT_EQ(root_of_kvstore_mem_trackers->get(), str_key.dataSize() + str_val_default.size());
}
ASSERT_EQ(root_of_kvstore_mem_trackers->get(), 0);