From 34450c00aa881e635fad57bc9e6ee108a5ef85e6 Mon Sep 17 00:00:00 2001 From: abmdocrt Date: Tue, 23 Sep 2025 11:03:28 +0800 Subject: [PATCH 1/4] [Enhancement](Snapshot) Add some configs for snapshot (#4675) --- cloud/src/common/config.h | 4 + .../meta-service/meta_service_resource.cpp | 27 ++- cloud/test/meta_service_test.cpp | 217 ++++++++++++++++++ 3 files changed, 242 insertions(+), 6 deletions(-) diff --git a/cloud/src/common/config.h b/cloud/src/common/config.h index 5ce674a42acfa4..0faaf6f7bd4a6a 100644 --- a/cloud/src/common/config.h +++ b/cloud/src/common/config.h @@ -366,4 +366,8 @@ CONF_mBool(enable_logging_conflict_keys, "false"); // Default is 1 hour (3600 seconds). CONF_Int64(prune_aborted_snapshot_seconds, "3600"); // 1h +// Snapshot configuration limits +CONF_Int32(snapshot_min_interval_seconds, "3600"); // 1h min interval limit +CONF_Int32(snapshot_max_reserved_num, "35"); // max reserved snapshots limit + } // namespace doris::cloud::config diff --git a/cloud/src/meta-service/meta_service_resource.cpp b/cloud/src/meta-service/meta_service_resource.cpp index 7b19dd435e9241..6ef92f6ddd2a84 100644 --- a/cloud/src/meta-service/meta_service_resource.cpp +++ b/cloud/src/meta-service/meta_service_resource.cpp @@ -1842,6 +1842,18 @@ std::pair handle_snapshot_switch(const std::string } if (value == "true") { instance->set_snapshot_switch_status(SNAPSHOT_SWITCH_ON); + + // Set default values when first enabling snapshot + if (!instance->has_snapshot_interval_seconds() || + instance->snapshot_interval_seconds() == 0) { + instance->set_snapshot_interval_seconds(3600); + LOG(INFO) << "Set default snapshot_interval_seconds to 3600 for instance " + << instance_id; + } + if (!instance->has_max_reserved_snapshot() || instance->max_reserved_snapshot() == 0) { + instance->set_max_reserved_snapshot(1); + LOG(INFO) << "Set default max_reserved_snapshots to 1 for instance " << instance_id; + } } else { instance->set_snapshot_switch_status(SNAPSHOT_SWITCH_OFF); } @@ -1862,9 +1874,11 @@ std::pair handle_max_reserved_snapshots( return std::make_pair(MetaServiceCode::INVALID_ARGUMENT, "max_reserved_snapshots must be non-negative, got: " + value); } - if (max_snapshots > 35) { + if (max_snapshots > config::snapshot_max_reserved_num) { return std::make_pair(MetaServiceCode::INVALID_ARGUMENT, - "max_reserved_snapshots too large, maximum is 35, got: " + value); + "max_reserved_snapshots too large, maximum is " + + std::to_string(config::snapshot_max_reserved_num) + + ", got: " + value); } } catch (const std::exception& e) { return std::make_pair(MetaServiceCode::INVALID_ARGUMENT, @@ -1886,10 +1900,11 @@ std::pair handle_snapshot_intervals(const std::str int intervals; try { intervals = std::stoi(value); - if (intervals < 3600) { - return std::make_pair( - MetaServiceCode::INVALID_ARGUMENT, - "snapshot_intervals too small, minimum is 3600 seconds, got: " + value); + if (intervals < config::snapshot_min_interval_seconds) { + return std::make_pair(MetaServiceCode::INVALID_ARGUMENT, + "snapshot_intervals too small, minimum is " + + std::to_string(config::snapshot_min_interval_seconds) + + " seconds, got: " + value); } } catch (const std::exception& e) { return std::make_pair(MetaServiceCode::INVALID_ARGUMENT, diff --git a/cloud/test/meta_service_test.cpp b/cloud/test/meta_service_test.cpp index f45158693dec86..91a51e85c4b5fb 100644 --- a/cloud/test/meta_service_test.cpp +++ b/cloud/test/meta_service_test.cpp @@ -11835,6 +11835,223 @@ TEST(MetaServiceTest, SetSnapshotPropertyTest) { } } +TEST(MetaServiceTest, SnapshotConfigLimitsTest) { + auto meta_service = get_meta_service(); + + // Create a test instance first + { + brpc::Controller cntl; + CreateInstanceRequest req; + req.set_instance_id("test_snapshot_config_instance"); + req.set_user_id("test_user"); + req.set_name("test_name"); + + auto* sp = SyncPoint::get_instance(); + sp->set_call_back("encrypt_ak_sk:get_encryption_key", [](auto&& args) { + auto* ret = try_any_cast(args[0]); + *ret = 0; + auto* key = try_any_cast(args[1]); + *key = "test"; + auto* key_id = try_any_cast(args[2]); + *key_id = 1; + }); + sp->enable_processing(); + CreateInstanceResponse res; + meta_service->create_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), + &req, &res, nullptr); + ASSERT_EQ(res.status().code(), MetaServiceCode::OK); + sp->clear_all_call_backs(); + sp->clear_trace(); + sp->disable_processing(); + } + + // Initialize snapshot switch status to OFF so we can test snapshot functionality + { + InstanceKeyInfo key_info {"test_snapshot_config_instance"}; + std::string key; + std::string val; + instance_key(key_info, &key); + std::unique_ptr txn; + ASSERT_EQ(meta_service->txn_kv()->create_txn(&txn), TxnErrorCode::TXN_OK); + ASSERT_EQ(txn->get(key, &val), TxnErrorCode::TXN_OK); + InstanceInfoPB instance; + instance.ParseFromString(val); + instance.set_snapshot_switch_status(SNAPSHOT_SWITCH_OFF); + val = instance.SerializeAsString(); + txn->put(key, val); + ASSERT_EQ(txn->commit(), TxnErrorCode::TXN_OK); + } + + // Enable snapshot for this instance + { + brpc::Controller cntl; + AlterInstanceRequest alter_req; + AlterInstanceResponse alter_res; + alter_req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); + alter_req.set_instance_id("test_snapshot_config_instance"); + (*alter_req.mutable_properties())["enabled"] = "true"; + + meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), + &alter_req, &alter_res, nullptr); + ASSERT_EQ(alter_res.status().code(), MetaServiceCode::OK); + } + + // Save original config values + int32_t orig_min_interval = config::snapshot_min_interval_seconds; + int32_t orig_max_reserved = config::snapshot_max_reserved_num; + + // Temporarily modify config for testing + config::snapshot_min_interval_seconds = 1800; // 30 minutes + config::snapshot_max_reserved_num = 20; + + // Test case 1: interval below minimum limit + { + brpc::Controller cntl; + AlterInstanceRequest req; + AlterInstanceResponse res; + req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); + req.set_instance_id("test_snapshot_config_instance"); + (*req.mutable_properties())["snapshot_interval_seconds"] = "1799"; // Below min limit + + meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), + &req, &res, nullptr); + ASSERT_EQ(res.status().code(), MetaServiceCode::INVALID_ARGUMENT); + ASSERT_TRUE(res.status().msg().find("minimum is 1800") != std::string::npos); + } + + // Test case 2: interval within limits should succeed + { + brpc::Controller cntl; + AlterInstanceRequest req; + AlterInstanceResponse res; + req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); + req.set_instance_id("test_snapshot_config_instance"); + (*req.mutable_properties())["snapshot_interval_seconds"] = "3600"; // Within limits + + meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), + &req, &res, nullptr); + ASSERT_EQ(res.status().code(), MetaServiceCode::OK); + } + + // Test case 3: max_reserved_snapshots above maximum limit + { + brpc::Controller cntl; + AlterInstanceRequest req; + AlterInstanceResponse res; + req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); + req.set_instance_id("test_snapshot_config_instance"); + (*req.mutable_properties())["max_reserved_snapshots"] = "21"; // Above max limit + + meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), + &req, &res, nullptr); + ASSERT_EQ(res.status().code(), MetaServiceCode::INVALID_ARGUMENT); + ASSERT_TRUE(res.status().msg().find("maximum is 20") != std::string::npos); + } + + // Test case 4: max_reserved_snapshots within limits should succeed + { + brpc::Controller cntl; + AlterInstanceRequest req; + AlterInstanceResponse res; + req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); + req.set_instance_id("test_snapshot_config_instance"); + (*req.mutable_properties())["max_reserved_snapshots"] = "10"; // Within limits + + meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), + &req, &res, nullptr); + ASSERT_EQ(res.status().code(), MetaServiceCode::OK); + } + + // Restore original config values + config::snapshot_min_interval_seconds = orig_min_interval; + config::snapshot_max_reserved_num = orig_max_reserved; +} + +TEST(MetaServiceTest, SnapshotDefaultValuesTest) { + auto meta_service = get_meta_service(); + + // Create a test instance first + { + brpc::Controller cntl; + CreateInstanceRequest req; + req.set_instance_id("test_instance"); + req.set_user_id("test_user"); + req.set_name("test_name"); + + auto* sp = SyncPoint::get_instance(); + sp->set_call_back("encrypt_ak_sk:get_encryption_key", [](auto&& args) { + auto* ret = try_any_cast(args[0]); + *ret = 0; + auto* key = try_any_cast(args[1]); + *key = "test"; + auto* key_id = try_any_cast(args[2]); + *key_id = 1; + }); + sp->set_call_back("get_instance_id", [&](auto&& args) { + auto* ret = try_any_cast_ret(args); + ret->first = "test_instance"; + ret->second = true; + }); + sp->enable_processing(); + CreateInstanceResponse res; + meta_service->create_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), + &req, &res, nullptr); + ASSERT_EQ(res.status().code(), MetaServiceCode::OK); + sp->clear_all_call_backs(); + sp->clear_trace(); + sp->disable_processing(); + } + + // Initialize snapshot switch status to OFF so we can test snapshot functionality + { + InstanceKeyInfo key_info {"test_instance"}; + std::string key; + std::string val; + instance_key(key_info, &key); + std::unique_ptr txn; + ASSERT_EQ(meta_service->txn_kv()->create_txn(&txn), TxnErrorCode::TXN_OK); + ASSERT_EQ(txn->get(key, &val), TxnErrorCode::TXN_OK); + InstanceInfoPB instance; + instance.ParseFromString(val); + instance.set_snapshot_switch_status(SNAPSHOT_SWITCH_OFF); + val = instance.SerializeAsString(); + txn->put(key, val); + ASSERT_EQ(txn->commit(), TxnErrorCode::TXN_OK); + } + + // First enable snapshot - should set default values + { + brpc::Controller cntl; + AlterInstanceRequest req; + AlterInstanceResponse res; + req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); + req.set_instance_id("test_instance"); + (*req.mutable_properties())["enabled"] = "true"; + + meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), + &req, &res, nullptr); + ASSERT_EQ(res.status().code(), MetaServiceCode::OK); + } + + // Verify default values were set by checking instance + { + brpc::Controller cntl; + GetInstanceRequest req; + GetInstanceResponse res; + req.set_instance_id("test_instance"); + req.set_cloud_unique_id("test_snapshot_defaults_unique_id"); + + meta_service->get_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), + &req, &res, nullptr); + ASSERT_EQ(res.status().code(), MetaServiceCode::OK); + + const auto& instance = res.instance(); + ASSERT_EQ(instance.snapshot_switch_status(), SnapshotSwitchStatus::SNAPSHOT_SWITCH_ON); + ASSERT_EQ(instance.snapshot_interval_seconds(), config::snapshot_min_interval_seconds); + ASSERT_EQ(instance.max_reserved_snapshot(), 1); + } +} + TEST(MetaServiceTest, CreateTabletIdempotentAndHandlingError) { DORIS_CLOUD_DEFER { SyncPoint::get_instance()->clear_all_call_backs(); From 428f6aa5eb0f4949ec1931996584c7545c2e2c91 Mon Sep 17 00:00:00 2001 From: Yukang-Lian Date: Tue, 23 Sep 2025 17:25:44 +0800 Subject: [PATCH 2/4] 1 --- cloud/src/meta-service/meta_service_http.cpp | 34 ++++++++++---- .../meta-service/meta_service_resource.cpp | 46 +++++++++++-------- .../meta-service/meta_service_snapshot.cpp | 20 ++++---- cloud/src/snapshot/snapshot_manager.cpp | 3 +- cloud/src/snapshot/snapshot_manager.h | 3 +- cloud/test/meta_service_test.cpp | 25 ++++++---- gensrc/proto/cloud.proto | 1 + 7 files changed, 84 insertions(+), 48 deletions(-) diff --git a/cloud/src/meta-service/meta_service_http.cpp b/cloud/src/meta-service/meta_service_http.cpp index c6608e813a94e4..a6d73aac903d32 100644 --- a/cloud/src/meta-service/meta_service_http.cpp +++ b/cloud/src/meta-service/meta_service_http.cpp @@ -687,12 +687,25 @@ static HttpResponse process_set_snapshot_property(MetaServiceImpl* service, static HttpResponse process_set_multi_version_status(MetaServiceImpl* service, brpc::Controller* ctrl) { auto& uri = ctrl->http_request().uri(); - std::string cloud_unique_id(http_query(uri, "cloud_unique_id")); std::string instance_id(http_query(uri, "instance_id")); + std::string cloud_unique_id(http_query(uri, "cloud_unique_id")); std::string multi_version_status_str(http_query(uri, "multi_version_status")); - if (cloud_unique_id.empty() || instance_id.empty() || multi_version_status_str.empty()) { - return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, "missing required arguments"); + // Prefer instance_id if provided, fallback to cloud_unique_id + if (instance_id.empty()) { + if (cloud_unique_id.empty()) { + return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, + "either instance_id or cloud_unique_id must be provided"); + } + instance_id = get_instance_id(service->resource_mgr(), cloud_unique_id); + if (instance_id.empty()) { + return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, "invalid cloud_unique_id"); + } + } + + if (multi_version_status_str.empty()) { + return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, + "multi_version_status is required"); } // Parse multi_version_status from string to enum @@ -716,8 +729,9 @@ static HttpResponse process_set_multi_version_status(MetaServiceImpl* service, "MULTI_VERSION_WRITE_ONLY, MULTI_VERSION_READ_WRITE, MULTI_VERSION_ENABLED"); } // Call snapshot manager directly - auto [code, msg] = service->snapshot_manager()->set_multi_version_status( - instance_id, cloud_unique_id, multi_version_status); + auto [code, msg] = service->snapshot_manager()->set_multi_version_status(instance_id, + multi_version_status); + return http_json_reply(code, msg); } @@ -751,19 +765,19 @@ static HttpResponse process_get_snapshot_property(MetaServiceImpl* service, std::string switch_status; switch (instance.snapshot_switch_status()) { case SNAPSHOT_SWITCH_DISABLED: - switch_status = "disabled"; + switch_status = "UNSUPPORTED"; break; case SNAPSHOT_SWITCH_OFF: - switch_status = "false"; + switch_status = "DISABLED"; break; case SNAPSHOT_SWITCH_ON: - switch_status = "true"; + switch_status = "ENABLED"; break; default: - switch_status = "unknown"; + switch_status = "UNKNOWN"; break; } - properties.AddMember("enabled", rapidjson::Value(switch_status.c_str(), allocator), allocator); + properties.AddMember("status", rapidjson::Value(switch_status.c_str(), allocator), allocator); // Max reserved snapshots if (instance.has_max_reserved_snapshot()) { diff --git a/cloud/src/meta-service/meta_service_resource.cpp b/cloud/src/meta-service/meta_service_resource.cpp index 6ef92f6ddd2a84..0701cc1db760a0 100644 --- a/cloud/src/meta-service/meta_service_resource.cpp +++ b/cloud/src/meta-service/meta_service_resource.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -47,7 +48,7 @@ using namespace std::chrono; namespace { constexpr char pattern_str[] = "^[a-zA-Z][0-9a-zA-Z_]*$"; -constexpr char SNAPSHOT_ENABLED_KEY[] = "enabled"; +constexpr char SNAPSHOT_STATUS_KEY[] = "status"; constexpr char SNAPSHOT_MAX_RESERVED_KEY[] = "max_reserved_snapshots"; constexpr char SNAPSHOT_INTERVAL_SECONDS_KEY[] = "snapshot_interval_seconds"; @@ -1820,30 +1821,40 @@ std::pair handle_snapshot_switch(const std::string const std::string& key, const std::string& value, InstanceInfoPB* instance) { - if (value != "true" && value != "false") { + // Only allow "ENABLED" and "DISABLED" values (case insensitive) + std::string value_upper = value; + std::ranges::transform(value_upper, value_upper.begin(), ::toupper); + + if (value_upper != "ENABLED" && value_upper != "DISABLED") { return std::make_pair(MetaServiceCode::INVALID_ARGUMENT, "Invalid value for enabled property: " + value + - ", expected 'true' or 'false'" + + ", expected 'ENABLED' or 'DISABLED' (case insensitive)" + ", instance_id: " + instance_id); } + + // Check if snapshot is not ready (UNSUPPORTED state) if (instance->snapshot_switch_status() == SNAPSHOT_SWITCH_DISABLED) { return std::make_pair(MetaServiceCode::INVALID_ARGUMENT, "Snapshot not ready, instance_id: " + instance_id); } - if (value == "true" && instance->snapshot_switch_status() == SNAPSHOT_SWITCH_ON) { - return std::make_pair( - MetaServiceCode::INVALID_ARGUMENT, - "Snapshot is already set to SNAPSHOT_SWITCH_ON, instance_id: " + instance_id); - } - if (value == "false" && instance->snapshot_switch_status() == SNAPSHOT_SWITCH_OFF) { + + // Determine target status + SnapshotSwitchStatus target_status = + (value_upper == "ENABLED") ? SNAPSHOT_SWITCH_ON : SNAPSHOT_SWITCH_OFF; + + // Check if the status is already set to the target value + if (instance->snapshot_switch_status() == target_status) { + std::string status_name = (target_status == SNAPSHOT_SWITCH_ON) ? "ENABLED" : "DISABLED"; return std::make_pair( MetaServiceCode::INVALID_ARGUMENT, - "Snapshot is already set to SNAPSHOT_SWITCH_OFF, instance_id: " + instance_id); + "Snapshot is already set to " + status_name + ", instance_id: " + instance_id); } - if (value == "true") { - instance->set_snapshot_switch_status(SNAPSHOT_SWITCH_ON); - // Set default values when first enabling snapshot + // Set the new status + instance->set_snapshot_switch_status(target_status); + + // Set default values when first enabling snapshot + if (target_status == SNAPSHOT_SWITCH_ON) { if (!instance->has_snapshot_interval_seconds() || instance->snapshot_interval_seconds() == 0) { instance->set_snapshot_interval_seconds(3600); @@ -1854,8 +1865,6 @@ std::pair handle_snapshot_switch(const std::string instance->set_max_reserved_snapshot(1); LOG(INFO) << "Set default max_reserved_snapshots to 1 for instance " << instance_id; } - } else { - instance->set_snapshot_switch_status(SNAPSHOT_SWITCH_OFF); } std::string msg = "Set snapshot enabled to " + value + " for instance " + instance_id; @@ -2112,8 +2121,8 @@ void MetaServiceImpl::alter_instance(google::protobuf::RpcController* controller * Handle SET_SNAPSHOT_PROPERTY operation - configures snapshot-related properties for an instance. * * Supported property keys and their expected values: - * - "enabled": "true" | "false" - * Controls whether snapshot functionality is enabled for the instance + * - "status": "UNSUPPORTED" | "ENABLED" | "DISABLED" + * Controls the snapshot functionality status for the instance * * - "max_reserved_snapshots": numeric string (0-35) * Sets the maximum number of snapshots to retain for the instance @@ -2139,7 +2148,7 @@ void MetaServiceImpl::alter_instance(google::protobuf::RpcController* controller std::pair result; - if (key == SNAPSHOT_ENABLED_KEY) { + if (key == SNAPSHOT_STATUS_KEY) { result = handle_snapshot_switch(request->instance_id(), key, value, instance); } else if (key == SNAPSHOT_MAX_RESERVED_KEY) { result = handle_max_reserved_snapshots(request->instance_id(), key, value, @@ -4292,6 +4301,7 @@ void MetaServiceImpl::get_cluster_status(google::protobuf::RpcController* contro void notify_refresh_instance(std::shared_ptr txn_kv, const std::string& instance_id, KVStats* stats) { LOG(INFO) << "begin notify_refresh_instance"; + TEST_SYNC_POINT_RETURN_WITH_VOID("notify_refresh_instance_return"); std::unique_ptr txn; TxnErrorCode err = txn_kv->create_txn(&txn); if (err != TxnErrorCode::TXN_OK) { diff --git a/cloud/src/meta-service/meta_service_snapshot.cpp b/cloud/src/meta-service/meta_service_snapshot.cpp index 93ab49a342d812..2ed22959ebfa96 100644 --- a/cloud/src/meta-service/meta_service_snapshot.cpp +++ b/cloud/src/meta-service/meta_service_snapshot.cpp @@ -101,16 +101,20 @@ void MetaServiceImpl::list_snapshot(::google::protobuf::RpcController* controlle ListSnapshotResponse* response, ::google::protobuf::Closure* done) { RPC_PREPROCESS(list_snapshot, get, put, del); - if (!request->has_cloud_unique_id() || request->cloud_unique_id().empty()) { - code = MetaServiceCode::INVALID_ARGUMENT; - msg = "cloud_unique_id not set"; - return; - } - instance_id = get_instance_id(resource_mgr_, request->cloud_unique_id()); - if (instance_id.empty()) { + // Prefer instance_id if provided, fallback to cloud_unique_id + if (request->has_instance_id() && !request->instance_id().empty()) { + instance_id = request->instance_id(); + } else if (request->has_cloud_unique_id() && !request->cloud_unique_id().empty()) { + instance_id = get_instance_id(resource_mgr_, request->cloud_unique_id()); + if (instance_id.empty()) { + code = MetaServiceCode::INVALID_ARGUMENT; + msg = "empty instance_id"; + return; + } + } else { code = MetaServiceCode::INVALID_ARGUMENT; - msg = "empty instance_id"; + msg = "either instance_id or cloud_unique_id must be provided"; return; } RPC_RATE_LIMIT(list_snapshot); diff --git a/cloud/src/snapshot/snapshot_manager.cpp b/cloud/src/snapshot/snapshot_manager.cpp index 5f72076a211a60..3c8b6684c906bf 100644 --- a/cloud/src/snapshot/snapshot_manager.cpp +++ b/cloud/src/snapshot/snapshot_manager.cpp @@ -64,8 +64,7 @@ void SnapshotManager::clone_instance(const CloneInstanceRequest& request, } std::pair SnapshotManager::set_multi_version_status( - std::string_view instance_id, std::string_view cloud_unique_id, - MultiVersionStatus multi_version_status) { + std::string_view instance_id, MultiVersionStatus multi_version_status) { return {MetaServiceCode::UNDEFINED_ERR, "Not implemented"}; } diff --git a/cloud/src/snapshot/snapshot_manager.h b/cloud/src/snapshot/snapshot_manager.h index 7055fef7579ce5..32d5a5fd67445c 100644 --- a/cloud/src/snapshot/snapshot_manager.h +++ b/cloud/src/snapshot/snapshot_manager.h @@ -48,8 +48,7 @@ class SnapshotManager { CloneInstanceResponse* response); virtual std::pair set_multi_version_status( - std::string_view instance_id, std::string_view cloud_unique_id, - MultiVersionStatus multi_version_status); + std::string_view instance_id, MultiVersionStatus multi_version_status); virtual int check_snapshots(InstanceChecker* checker); diff --git a/cloud/test/meta_service_test.cpp b/cloud/test/meta_service_test.cpp index 91a51e85c4b5fb..1bff9e87be47ae 100644 --- a/cloud/test/meta_service_test.cpp +++ b/cloud/test/meta_service_test.cpp @@ -11546,6 +11546,15 @@ TEST(MetaServiceTest, SetSnapshotPropertyTest) { sp->disable_processing(); } + auto* sp = SyncPoint::get_instance(); + sp->set_call_back("notify_refresh_instance_return", + [](auto&& args) { *try_any_cast(args.back()) = true; }); + sp->enable_processing(); + DORIS_CLOUD_DEFER { + SyncPoint::get_instance()->clear_all_call_backs(); + SyncPoint::get_instance()->disable_processing(); + }; + // Test case 1: Snapshot not ready (SNAPSHOT_SWITCH_DISABLED) - should fail { brpc::Controller cntl; @@ -11553,7 +11562,7 @@ TEST(MetaServiceTest, SetSnapshotPropertyTest) { AlterInstanceResponse res; req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); req.set_instance_id("test_snapshot_instance"); - (*req.mutable_properties())["enabled"] = "true"; + (*req.mutable_properties())["status"] = "ENABLED"; meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), &req, &res, nullptr); @@ -11585,7 +11594,7 @@ TEST(MetaServiceTest, SetSnapshotPropertyTest) { AlterInstanceResponse res; req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); req.set_instance_id("test_snapshot_instance"); - (*req.mutable_properties())["enabled"] = "true"; + (*req.mutable_properties())["status"] = "ENABLED"; meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), &req, &res, nullptr); @@ -11599,7 +11608,7 @@ TEST(MetaServiceTest, SetSnapshotPropertyTest) { AlterInstanceResponse res; req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); req.set_instance_id("test_snapshot_instance"); - (*req.mutable_properties())["enabled"] = "false"; + (*req.mutable_properties())["status"] = "DISABLED"; meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), &req, &res, nullptr); @@ -11613,7 +11622,7 @@ TEST(MetaServiceTest, SetSnapshotPropertyTest) { AlterInstanceResponse res; req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); req.set_instance_id("test_snapshot_instance"); - (*req.mutable_properties())["enabled"] = "invalid"; + (*req.mutable_properties())["status"] = "invalid"; meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), &req, &res, nullptr); @@ -11795,7 +11804,7 @@ TEST(MetaServiceTest, SetSnapshotPropertyTest) { AlterInstanceResponse res; req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); req.set_instance_id("test_snapshot_instance"); - (*req.mutable_properties())["enabled"] = "true"; + (*req.mutable_properties())["status"] = "ENABLED"; (*req.mutable_properties())["max_reserved_snapshots"] = "20"; (*req.mutable_properties())["snapshot_interval_seconds"] = "12000"; @@ -11811,7 +11820,7 @@ TEST(MetaServiceTest, SetSnapshotPropertyTest) { AlterInstanceResponse res; req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); req.set_instance_id("test_snapshot_instance"); - (*req.mutable_properties())["enabled"] = "true"; + (*req.mutable_properties())["status"] = "ENABLED"; (*req.mutable_properties())["invalid_property"] = "value"; (*req.mutable_properties())["max_reserved_snapshots"] = "10"; @@ -11827,7 +11836,7 @@ TEST(MetaServiceTest, SetSnapshotPropertyTest) { AlterInstanceResponse res; req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); req.set_instance_id("non_existent_instance"); - (*req.mutable_properties())["enabled"] = "true"; + (*req.mutable_properties())["status"] = "ENABLED"; meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), &req, &res, nullptr); @@ -12026,7 +12035,7 @@ TEST(MetaServiceTest, SnapshotDefaultValuesTest) { AlterInstanceResponse res; req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); req.set_instance_id("test_instance"); - (*req.mutable_properties())["enabled"] = "true"; + (*req.mutable_properties())["status"] = "ENABLED"; meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), &req, &res, nullptr); diff --git a/gensrc/proto/cloud.proto b/gensrc/proto/cloud.proto index 380c9119d35340..253ec7318b5adb 100644 --- a/gensrc/proto/cloud.proto +++ b/gensrc/proto/cloud.proto @@ -1998,6 +1998,7 @@ message ListSnapshotRequest { optional string required_snapshot_id = 2; // Return all snapshots if not set. optional bool include_aborted = 3; optional string request_ip = 4; + optional string instance_id = 5; } message ListSnapshotResponse { From deb44ad4fda5ed17c077ceff0d482b976a89b56f Mon Sep 17 00:00:00 2001 From: Yukang-Lian Date: Wed, 24 Sep 2025 16:04:00 +0800 Subject: [PATCH 3/4] 2 --- cloud/src/meta-service/meta_service_http.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/cloud/src/meta-service/meta_service_http.cpp b/cloud/src/meta-service/meta_service_http.cpp index a6d73aac903d32..54588ff49809ef 100644 --- a/cloud/src/meta-service/meta_service_http.cpp +++ b/cloud/src/meta-service/meta_service_http.cpp @@ -693,14 +693,7 @@ static HttpResponse process_set_multi_version_status(MetaServiceImpl* service, // Prefer instance_id if provided, fallback to cloud_unique_id if (instance_id.empty()) { - if (cloud_unique_id.empty()) { - return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, - "either instance_id or cloud_unique_id must be provided"); - } - instance_id = get_instance_id(service->resource_mgr(), cloud_unique_id); - if (instance_id.empty()) { - return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, "invalid cloud_unique_id"); - } + return http_json_reply(MetaServiceCode::INVALID_ARGUMENT, "empty instance id"); } if (multi_version_status_str.empty()) { From 34780c63baea042f074604a1f99cb95c3b0bfb73 Mon Sep 17 00:00:00 2001 From: Yukang-Lian Date: Wed, 24 Sep 2025 16:42:55 +0800 Subject: [PATCH 4/4] 3 --- cloud/test/meta_service_test.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/cloud/test/meta_service_test.cpp b/cloud/test/meta_service_test.cpp index 1bff9e87be47ae..70100044d7a523 100644 --- a/cloud/test/meta_service_test.cpp +++ b/cloud/test/meta_service_test.cpp @@ -11874,6 +11874,15 @@ TEST(MetaServiceTest, SnapshotConfigLimitsTest) { sp->disable_processing(); } + auto* sp = SyncPoint::get_instance(); + sp->set_call_back("notify_refresh_instance_return", + [](auto&& args) { *try_any_cast(args.back()) = true; }); + sp->enable_processing(); + DORIS_CLOUD_DEFER { + SyncPoint::get_instance()->clear_all_call_backs(); + SyncPoint::get_instance()->disable_processing(); + }; + // Initialize snapshot switch status to OFF so we can test snapshot functionality { InstanceKeyInfo key_info {"test_snapshot_config_instance"}; @@ -11898,7 +11907,7 @@ TEST(MetaServiceTest, SnapshotConfigLimitsTest) { AlterInstanceResponse alter_res; alter_req.set_op(AlterInstanceRequest::SET_SNAPSHOT_PROPERTY); alter_req.set_instance_id("test_snapshot_config_instance"); - (*alter_req.mutable_properties())["enabled"] = "true"; + (*alter_req.mutable_properties())["status"] = "enabled"; meta_service->alter_instance(reinterpret_cast<::google::protobuf::RpcController*>(&cntl), &alter_req, &alter_res, nullptr);