From c55a5f3545ac558adebbe72927a6c227348f39de Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Fri, 8 Nov 2024 11:14:35 +0800 Subject: [PATCH 01/25] fix(duplication): dup would get stuck in DS_PREPARE indefinitely once some error occurred during creating follower table --- src/common/duplication_common.cpp | 6 ++++ src/common/duplication_common.h | 3 ++ .../duplication/meta_duplication_service.cpp | 28 +++++++++++++++++-- src/meta/server_state.cpp | 12 +++++++- 4 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/common/duplication_common.cpp b/src/common/duplication_common.cpp index 4dc5323c65..95dc87e91e 100644 --- a/src/common/duplication_common.cpp +++ b/src/common/duplication_common.cpp @@ -61,6 +61,12 @@ const std::string duplication_constants::kDuplicationEnvMasterMetasKey /*NOLINT* "duplication.master_metas"; const std::string duplication_constants::kDuplicationEnvMasterAppNameKey /*NOLINT*/ = "duplication.master_app_name"; +const std::string duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey /*NOLINT*/ = + "duplication.master_create_follower_app_status"; +const std::string duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating /*NOLINT*/ = + "creating"; +const std::string duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated /*NOLINT*/ = + "created"; /*extern*/ const char *duplication_status_to_string(duplication_status::type status) { diff --git a/src/common/duplication_common.h b/src/common/duplication_common.h index 8613357338..e6f072af64 100644 --- a/src/common/duplication_common.h +++ b/src/common/duplication_common.h @@ -84,6 +84,9 @@ struct duplication_constants const static std::string kDuplicationEnvMasterClusterKey; const static std::string kDuplicationEnvMasterMetasKey; const static std::string kDuplicationEnvMasterAppNameKey; + const static std::string kDuplicationEnvMasterCreateFollowerAppStatusKey; + const static std::string kDuplicationEnvMasterCreateFollowerAppStatusCreating; + const static std::string kDuplicationEnvMasterCreateFollowerAppStatusCreated; }; USER_DEFINED_ENUM_FORMATTER(duplication_fail_mode::type) diff --git a/src/meta/duplication/meta_duplication_service.cpp b/src/meta/duplication/meta_duplication_service.cpp index 66cd2ed125..24caab1ff7 100644 --- a/src/meta/duplication/meta_duplication_service.cpp +++ b/src/meta/duplication/meta_duplication_service.cpp @@ -463,6 +463,18 @@ void meta_duplication_service::duplication_sync(duplication_sync_rpc rpc) void meta_duplication_service::create_follower_app_for_duplication( const std::shared_ptr &dup, const std::shared_ptr &app) +{ + do_create_follower_app_for_duplication(dup, app, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating, std::bind(&meta_duplication_service::on_follower_app_creating_for_duplication, this, dup, std::placeholders::_1, std::placeholders::_2)); +} + +void meta_duplication_service::mark_follower_app_created_for_duplication( + const std::shared_ptr &dup, const std::shared_ptr &app) +{ + do_create_follower_app_for_duplication(dup, app, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated, std::bind(&meta_duplication_service::on_follower_app_created_for_duplication, this, dup, std::placeholders::_1, std::placeholders::_2)); +} + +void meta_duplication_service::do_create_follower_app_for_duplication( + const std::shared_ptr &dup, const std::shared_ptr &app, const std::string &create_status, std::function create_callback) { configuration_create_app_request request; request.app_name = dup->remote_app_name; @@ -483,6 +495,8 @@ void meta_duplication_service::create_follower_app_for_duplication( _meta_svc->get_meta_list_string()); request.options.envs.emplace(duplication_constants::kDuplicationEnvMasterAppNameKey, app->app_name); + request.options.envs.emplace(duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + create_status); host_port meta_servers; meta_servers.assign_group(dup->remote_cluster_name.c_str()); @@ -494,7 +508,10 @@ void meta_duplication_service::create_follower_app_for_duplication( dsn::dns_resolver::instance().resolve_address(meta_servers), msg, _meta_svc->tracker(), - [dup, this](error_code err, configuration_create_app_response &&resp) mutable { + std::move(create_callback)); +} + +void meta_duplication_service::on_follower_app_creating_for_duplication(const std::shared_ptr &dup, error_code err, configuration_create_app_response &&resp) { FAIL_POINT_INJECT_NOT_RETURN_F("update_app_request_ok", [&err](std::string_view) -> void { err = ERR_OK; }); @@ -528,7 +545,14 @@ void meta_duplication_service::create_follower_app_for_duplication( create_err, update_err); } - }); +} + +void meta_duplication_service::on_follower_app_created_for_duplication(const std::shared_ptr &dup, error_code err, configuration_create_app_response &&resp) { + if (err != ERR_OK || resp.err != ERR_OK) { + return; + } + + check_follower_app_if_create_completed(dup); } namespace { diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index 68bc786187..a908f8a9e0 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -1127,7 +1127,7 @@ void server_state::create_app(dsn::message_ex *msg) switch (app->status) { case app_status::AS_AVAILABLE: if (!request.options.success_if_exist) { - response.err = ERR_APP_EXIST; + response.err = is_follower_app_creating(*app) ? ERR_OK : ERR_APP_EXIST; } else if (!option_match_check(request.options, *app)) { response.err = ERR_INVALID_PARAMETERS; } else { @@ -3050,6 +3050,16 @@ bool validate_target_max_replica_count_internal(int32_t max_replica_count, return true; } +bool is_follower_app_creating(const dsn::replication::app_state &app) +{ + const auto &iter = app.envs.find(dsn::replication::duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); + if (iter == app.envs.end()) { + return false; + } + + return iter->second == dsn::replication::duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating; +} + } // anonymous namespace bool server_state::validate_target_max_replica_count(int32_t max_replica_count, From c4ba8fc5d73691f030a5fd4aedb576b20b6d1791 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Fri, 8 Nov 2024 11:53:37 +0800 Subject: [PATCH 02/25] format and fix --- src/common/duplication_common.cpp | 14 ++- .../duplication/meta_duplication_service.cpp | 106 +++++++++++------- .../duplication/meta_duplication_service.h | 14 +++ src/meta/server_state.cpp | 26 +++-- 4 files changed, 103 insertions(+), 57 deletions(-) diff --git a/src/common/duplication_common.cpp b/src/common/duplication_common.cpp index 95dc87e91e..7f44bf7412 100644 --- a/src/common/duplication_common.cpp +++ b/src/common/duplication_common.cpp @@ -61,12 +61,14 @@ const std::string duplication_constants::kDuplicationEnvMasterMetasKey /*NOLINT* "duplication.master_metas"; const std::string duplication_constants::kDuplicationEnvMasterAppNameKey /*NOLINT*/ = "duplication.master_app_name"; -const std::string duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey /*NOLINT*/ = - "duplication.master_create_follower_app_status"; -const std::string duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating /*NOLINT*/ = - "creating"; -const std::string duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated /*NOLINT*/ = - "created"; +const std::string duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey /*NOLINT*/ + = "duplication.master_create_follower_app_status"; +const std::string + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating /*NOLINT*/ + = "creating"; +const std::string + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated /*NOLINT*/ + = "created"; /*extern*/ const char *duplication_status_to_string(duplication_status::type status) { diff --git a/src/meta/duplication/meta_duplication_service.cpp b/src/meta/duplication/meta_duplication_service.cpp index 24caab1ff7..71985714eb 100644 --- a/src/meta/duplication/meta_duplication_service.cpp +++ b/src/meta/duplication/meta_duplication_service.cpp @@ -464,17 +464,36 @@ void meta_duplication_service::duplication_sync(duplication_sync_rpc rpc) void meta_duplication_service::create_follower_app_for_duplication( const std::shared_ptr &dup, const std::shared_ptr &app) { - do_create_follower_app_for_duplication(dup, app, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating, std::bind(&meta_duplication_service::on_follower_app_creating_for_duplication, this, dup, std::placeholders::_1, std::placeholders::_2)); + do_create_follower_app_for_duplication( + dup, + app, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating, + std::bind(&meta_duplication_service::on_follower_app_creating_for_duplication, + this, + dup, + std::placeholders::_1, + std::placeholders::_2)); } void meta_duplication_service::mark_follower_app_created_for_duplication( const std::shared_ptr &dup, const std::shared_ptr &app) { - do_create_follower_app_for_duplication(dup, app, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated, std::bind(&meta_duplication_service::on_follower_app_created_for_duplication, this, dup, std::placeholders::_1, std::placeholders::_2)); + do_create_follower_app_for_duplication( + dup, + app, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated, + std::bind(&meta_duplication_service::on_follower_app_created_for_duplication, + this, + dup, + std::placeholders::_1, + std::placeholders::_2)); } void meta_duplication_service::do_create_follower_app_for_duplication( - const std::shared_ptr &dup, const std::shared_ptr &app, const std::string &create_status, std::function create_callback) + const std::shared_ptr &dup, + const std::shared_ptr &app, + const std::string &create_status, + std::function create_callback) { configuration_create_app_request request; request.app_name = dup->remote_app_name; @@ -495,8 +514,8 @@ void meta_duplication_service::do_create_follower_app_for_duplication( _meta_svc->get_meta_list_string()); request.options.envs.emplace(duplication_constants::kDuplicationEnvMasterAppNameKey, app->app_name); - request.options.envs.emplace(duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, - create_status); + request.options.envs.emplace( + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, create_status); host_port meta_servers; meta_servers.assign_group(dup->remote_cluster_name.c_str()); @@ -504,50 +523,55 @@ void meta_duplication_service::do_create_follower_app_for_duplication( dsn::message_ex *msg = dsn::message_ex::create_request(RPC_CM_CREATE_APP); dsn::marshall(msg, request); - rpc::call( - dsn::dns_resolver::instance().resolve_address(meta_servers), - msg, - _meta_svc->tracker(), - std::move(create_callback)); + rpc::call(dsn::dns_resolver::instance().resolve_address(meta_servers), + msg, + _meta_svc->tracker(), + std::move(create_callback)); } -void meta_duplication_service::on_follower_app_creating_for_duplication(const std::shared_ptr &dup, error_code err, configuration_create_app_response &&resp) { - FAIL_POINT_INJECT_NOT_RETURN_F("update_app_request_ok", - [&err](std::string_view) -> void { err = ERR_OK; }); +void meta_duplication_service::on_follower_app_creating_for_duplication( + const std::shared_ptr &dup, + error_code err, + configuration_create_app_response &&resp) +{ + FAIL_POINT_INJECT_NOT_RETURN_F("update_app_request_ok", + [&err](std::string_view) -> void { err = ERR_OK; }); - error_code create_err = err == ERR_OK ? resp.err : err; - FAIL_POINT_INJECT_NOT_RETURN_F( - "persist_dup_status_failed", - [&create_err](std::string_view) -> void { create_err = ERR_OK; }); + error_code create_err = err == ERR_OK ? resp.err : err; + FAIL_POINT_INJECT_NOT_RETURN_F( + "persist_dup_status_failed", + [&create_err](std::string_view) -> void { create_err = ERR_OK; }); - error_code update_err = ERR_NO_NEED_OPERATE; - if (create_err == ERR_OK) { - update_err = dup->alter_status(duplication_status::DS_APP); - } + error_code update_err = ERR_NO_NEED_OPERATE; + if (create_err == ERR_OK) { + update_err = dup->alter_status(duplication_status::DS_APP); + } - FAIL_POINT_INJECT_F("persist_dup_status_failed", - [](std::string_view) -> void { return; }); + FAIL_POINT_INJECT_F("persist_dup_status_failed", [](std::string_view) -> void { return; }); - if (update_err == ERR_OK) { - blob value = dup->to_json_blob(); - // Note: this function is `async`, it may not be persisted completed - // after executing, now using `_is_altering` to judge whether `updating` or - // `completed`, if `_is_altering`, dup->alter_status() will return `ERR_BUSY` - _meta_svc->get_meta_storage()->set_data(std::string(dup->store_path), - std::move(value), - [dup]() { dup->persist_status(); }); - } else { - LOG_ERROR("create follower app[{}.{}] to trigger duplicate checkpoint failed: " - "duplication_status = {}, create_err = {}, update_err = {}", - dup->remote_cluster_name, - dup->remote_app_name, - duplication_status_to_string(dup->status()), - create_err, - update_err); - } + if (update_err == ERR_OK) { + blob value = dup->to_json_blob(); + // Note: this function is `async`, it may not be persisted completed + // after executing, now using `_is_altering` to judge whether `updating` or + // `completed`, if `_is_altering`, dup->alter_status() will return `ERR_BUSY` + _meta_svc->get_meta_storage()->set_data( + std::string(dup->store_path), std::move(value), [dup]() { dup->persist_status(); }); + } else { + LOG_ERROR("create follower app[{}.{}] to trigger duplicate checkpoint failed: " + "duplication_status = {}, create_err = {}, update_err = {}", + dup->remote_cluster_name, + dup->remote_app_name, + duplication_status_to_string(dup->status()), + create_err, + update_err); + } } -void meta_duplication_service::on_follower_app_created_for_duplication(const std::shared_ptr &dup, error_code err, configuration_create_app_response &&resp) { +void meta_duplication_service::on_follower_app_created_for_duplication( + const std::shared_ptr &dup, + error_code err, + configuration_create_app_response &&resp) +{ if (err != ERR_OK || resp.err != ERR_OK) { return; } diff --git a/src/meta/duplication/meta_duplication_service.h b/src/meta/duplication/meta_duplication_service.h index 3f06d63265..9becf44576 100644 --- a/src/meta/duplication/meta_duplication_service.h +++ b/src/meta/duplication/meta_duplication_service.h @@ -103,6 +103,20 @@ class meta_duplication_service void create_follower_app_for_duplication(const std::shared_ptr &dup, const std::shared_ptr &app); + void do_create_follower_app_for_duplication( + const std::shared_ptr &dup, + const std::shared_ptr &app, + const std::string &create_status, + std::function create_callback); + void mark_follower_app_created_for_duplication(const std::shared_ptr &dup, + const std::shared_ptr &app); + void on_follower_app_creating_for_duplication(const std::shared_ptr &dup, + error_code err, + configuration_create_app_response &&resp); + void on_follower_app_created_for_duplication(const std::shared_ptr &dup, + error_code err, + configuration_create_app_response &&resp); + void check_follower_app_if_create_completed(const std::shared_ptr &dup); // Get zk path for duplication. diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index a908f8a9e0..064b570034 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -1078,6 +1078,22 @@ void server_state::do_app_create(std::shared_ptr &app) app_dir, LPC_META_STATE_HIGH, on_create_app_root, value); } +namespace { + +bool is_follower_app_creating(const dsn::replication::app_state &app) +{ + const auto &iter = app.envs.find( + dsn::replication::duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); + if (iter == app.envs.end()) { + return false; + } + + return iter->second == dsn::replication::duplication_constants:: + kDuplicationEnvMasterCreateFollowerAppStatusCreating; +} + +} // anonymous namespace + void server_state::create_app(dsn::message_ex *msg) { configuration_create_app_request request; @@ -3050,16 +3066,6 @@ bool validate_target_max_replica_count_internal(int32_t max_replica_count, return true; } -bool is_follower_app_creating(const dsn::replication::app_state &app) -{ - const auto &iter = app.envs.find(dsn::replication::duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); - if (iter == app.envs.end()) { - return false; - } - - return iter->second == dsn::replication::duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating; -} - } // anonymous namespace bool server_state::validate_target_max_replica_count(int32_t max_replica_count, From 33db8c6c545b6ca19ad79a09227ae15ac095b906 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Fri, 8 Nov 2024 12:44:51 +0800 Subject: [PATCH 03/25] fix clang-tidy --- src/common/duplication_common.h | 1 + .../duplication/meta_duplication_service.cpp | 16 ++++----- src/rpc/rpc_stream.h | 6 ++-- src/utils/binary_writer.cpp | 36 +++++++++---------- src/utils/binary_writer.h | 7 ++-- src/utils/fmt_utils.h | 2 +- 6 files changed, 33 insertions(+), 35 deletions(-) diff --git a/src/common/duplication_common.h b/src/common/duplication_common.h index e6f072af64..84223c1f3d 100644 --- a/src/common/duplication_common.h +++ b/src/common/duplication_common.h @@ -91,5 +91,6 @@ struct duplication_constants USER_DEFINED_ENUM_FORMATTER(duplication_fail_mode::type) USER_DEFINED_ENUM_FORMATTER(duplication_status::type) + } // namespace replication } // namespace dsn diff --git a/src/meta/duplication/meta_duplication_service.cpp b/src/meta/duplication/meta_duplication_service.cpp index 71985714eb..d0c87272d8 100644 --- a/src/meta/duplication/meta_duplication_service.cpp +++ b/src/meta/duplication/meta_duplication_service.cpp @@ -468,11 +468,9 @@ void meta_duplication_service::create_follower_app_for_duplication( dup, app, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating, - std::bind(&meta_duplication_service::on_follower_app_creating_for_duplication, - this, - dup, - std::placeholders::_1, - std::placeholders::_2)); + [this, dup](error_code err, configuration_create_app_response &&resp) { + on_follower_app_creating_for_duplication(dup, err, std::move(resp)); + }); } void meta_duplication_service::mark_follower_app_created_for_duplication( @@ -482,11 +480,9 @@ void meta_duplication_service::mark_follower_app_created_for_duplication( dup, app, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated, - std::bind(&meta_duplication_service::on_follower_app_created_for_duplication, - this, - dup, - std::placeholders::_1, - std::placeholders::_2)); + [this, dup](error_code err, configuration_create_app_response &&resp) { + on_follower_app_created_for_duplication(dup, err, std::move(resp)); + }); } void meta_duplication_service::do_create_follower_app_for_duplication( diff --git a/src/rpc/rpc_stream.h b/src/rpc/rpc_stream.h index f9953e8d89..93074be7f2 100644 --- a/src/rpc/rpc_stream.h +++ b/src/rpc/rpc_stream.h @@ -104,16 +104,16 @@ class rpc_write_stream : public binary_writer } } - virtual ~rpc_write_stream() { flush(); } + ~rpc_write_stream() override { flush(); } - virtual void flush() override + void flush() override { binary_writer::flush(); commit_buffer(); } private: - virtual void create_new_buffer(size_t size, /*out*/ blob &bb) override + void create_new_buffer(size_t size, /*out*/ blob &bb) override { commit_buffer(); diff --git a/src/utils/binary_writer.cpp b/src/utils/binary_writer.cpp index 9234ce9140..e7fe886c89 100644 --- a/src/utils/binary_writer.cpp +++ b/src/utils/binary_writer.cpp @@ -32,33 +32,32 @@ #include "utils/blob.h" namespace dsn { -int binary_writer::_reserved_size_per_buffer_static = 256; -binary_writer::binary_writer(int reserveBufferSize) +const int binary_writer::kReservedSizePerBuffer = 256; + +binary_writer::binary_writer() : binary_writer(0) {} + +binary_writer::binary_writer(int reserved_buffer_size) + : _current_buffer(nullptr), + _current_offset(0), + _current_buffer_length(0), + _total_size(0), + _reserved_size_per_buffer((reserved_buffer_size == 0) ? kReservedSizePerBuffer + : reserved_buffer_size) { - _total_size = 0; _buffers.reserve(1); - _reserved_size_per_buffer = (reserveBufferSize == 0) ? _reserved_size_per_buffer_static - : reserveBufferSize; - _current_buffer = nullptr; - _current_offset = 0; - _current_buffer_length = 0; } binary_writer::binary_writer(blob &buffer) + : _buffers({buffer}), + _current_buffer(const_cast(buffer.data())), + _current_offset(0), + _current_buffer_length(buffer.length()), + _total_size(0), + _reserved_size_per_buffer(kReservedSizePerBuffer) { - _total_size = 0; - _buffers.reserve(1); - _reserved_size_per_buffer = _reserved_size_per_buffer_static; - - _buffers.push_back(buffer); - _current_buffer = (char *)buffer.data(); - _current_offset = 0; - _current_buffer_length = buffer.length(); } -binary_writer::~binary_writer() {} - void binary_writer::flush() { commit(); } void binary_writer::create_buffer(size_t size) @@ -200,4 +199,5 @@ bool binary_writer::backup(int count) _total_size -= count; return true; } + } // namespace dsn diff --git a/src/utils/binary_writer.h b/src/utils/binary_writer.h index 2640567f05..e857c55a76 100644 --- a/src/utils/binary_writer.h +++ b/src/utils/binary_writer.h @@ -40,9 +40,10 @@ namespace dsn { class binary_writer { public: - binary_writer(int reserved_buffer_size = 0); + binary_writer(); + binary_writer(int reserved_buffer_size); binary_writer(blob &buffer); - virtual ~binary_writer(); + virtual ~binary_writer() = default; virtual void flush(); @@ -95,7 +96,7 @@ class binary_writer int _total_size; int _reserved_size_per_buffer; - static int _reserved_size_per_buffer_static; + static const int kReservedSizePerBuffer; }; //--------------- inline implementation ------------------- diff --git a/src/utils/fmt_utils.h b/src/utils/fmt_utils.h index 9624bb881a..2d18201c16 100644 --- a/src/utils/fmt_utils.h +++ b/src/utils/fmt_utils.h @@ -26,4 +26,4 @@ } #define USER_DEFINED_ENUM_FORMATTER(type) \ - inline auto format_as(type e)->int { return e; } + inline int format_as(type e) { return e; } From 9e94f756547250af5d811340b871293a9caae691 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Fri, 8 Nov 2024 14:17:01 +0800 Subject: [PATCH 04/25] fix clang-tidy --- src/meta/duplication/meta_duplication_service.h | 2 ++ src/rpc/rpc_stream.h | 17 +++++++++++------ src/utils/binary_writer.cpp | 2 +- src/utils/binary_writer.h | 4 ++-- 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/meta/duplication/meta_duplication_service.h b/src/meta/duplication/meta_duplication_service.h index 9becf44576..1bc3edf7c4 100644 --- a/src/meta/duplication/meta_duplication_service.h +++ b/src/meta/duplication/meta_duplication_service.h @@ -18,6 +18,7 @@ #pragma once #include +#include #include #include #include @@ -36,6 +37,7 @@ class host_port; class zrwlock_nr; namespace replication { +class configuration_create_app_response; class duplication_confirm_entry; class duplication_query_request; class duplication_query_response; diff --git a/src/rpc/rpc_stream.h b/src/rpc/rpc_stream.h index 93074be7f2..023d6fb501 100644 --- a/src/rpc/rpc_stream.h +++ b/src/rpc/rpc_stream.h @@ -104,14 +104,14 @@ class rpc_write_stream : public binary_writer } } - ~rpc_write_stream() override { flush(); } - - void flush() override + ~rpc_write_stream() override { - binary_writer::flush(); - commit_buffer(); + // Avoid calling virtual functions in destructor. + flush_internal(); } + void flush() override { flush_internal(); } + private: void create_new_buffer(size_t size, /*out*/ blob &bb) override { @@ -127,7 +127,12 @@ class rpc_write_stream : public binary_writer _last_write_next_committed = false; } -private: + void flush_internal() + { + binary_writer::flush(); + commit_buffer(); + } + message_ex *_msg; bool _last_write_next_committed; int _last_write_next_total_size; diff --git a/src/utils/binary_writer.cpp b/src/utils/binary_writer.cpp index e7fe886c89..64049dfe6b 100644 --- a/src/utils/binary_writer.cpp +++ b/src/utils/binary_writer.cpp @@ -52,7 +52,7 @@ binary_writer::binary_writer(blob &buffer) : _buffers({buffer}), _current_buffer(const_cast(buffer.data())), _current_offset(0), - _current_buffer_length(buffer.length()), + _current_buffer_length(static_cast(buffer.length())), _total_size(0), _reserved_size_per_buffer(kReservedSizePerBuffer) { diff --git a/src/utils/binary_writer.h b/src/utils/binary_writer.h index e857c55a76..9fc957de76 100644 --- a/src/utils/binary_writer.h +++ b/src/utils/binary_writer.h @@ -41,8 +41,8 @@ class binary_writer { public: binary_writer(); - binary_writer(int reserved_buffer_size); - binary_writer(blob &buffer); + explicit binary_writer(int reserved_buffer_size); + explicit binary_writer(blob &buffer); virtual ~binary_writer() = default; virtual void flush(); From 156b18d1d020c4861573c5e1bc741e9be11b12fa Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Fri, 8 Nov 2024 15:05:46 +0800 Subject: [PATCH 05/25] fix clang-tidy --- .../duplication/meta_duplication_service.h | 2 +- .../test/meta_state/meta_state_service.cpp | 4 +++- src/rpc/rpc_stream.h | 4 ++++ src/utils/binary_writer.h | 3 +++ src/utils/ports.h | 18 +++++++++++++++--- 5 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/meta/duplication/meta_duplication_service.h b/src/meta/duplication/meta_duplication_service.h index 1bc3edf7c4..4f4b9af740 100644 --- a/src/meta/duplication/meta_duplication_service.h +++ b/src/meta/duplication/meta_duplication_service.h @@ -17,7 +17,7 @@ #pragma once -#include +#include #include #include #include diff --git a/src/meta/test/meta_state/meta_state_service.cpp b/src/meta/test/meta_state/meta_state_service.cpp index 7c1db06621..ae0d1f3f43 100644 --- a/src/meta/test/meta_state/meta_state_service.cpp +++ b/src/meta/test/meta_state/meta_state_service.cpp @@ -118,7 +118,9 @@ void provider_basic_test(const service_creator_func &service_creator, CHECK_EQ(0xdeadbeef, read_value); }) ->wait(); - writer = dsn::binary_writer(); + } + { + dsn::binary_writer writer; writer.write(0xbeefdead); service ->set_data( diff --git a/src/rpc/rpc_stream.h b/src/rpc/rpc_stream.h index 023d6fb501..0d85960315 100644 --- a/src/rpc/rpc_stream.h +++ b/src/rpc/rpc_stream.h @@ -136,6 +136,10 @@ class rpc_write_stream : public binary_writer message_ex *_msg; bool _last_write_next_committed; int _last_write_next_total_size; + + rpc_write_stream() = delete; }; + typedef ::dsn::ref_ptr rpc_write_stream_ptr; + } // namespace dsn diff --git a/src/utils/binary_writer.h b/src/utils/binary_writer.h index 9fc957de76..816bec18ee 100644 --- a/src/utils/binary_writer.h +++ b/src/utils/binary_writer.h @@ -97,6 +97,9 @@ class binary_writer int _total_size; int _reserved_size_per_buffer; static const int kReservedSizePerBuffer; + + DISALLOW_COPY_AND_ASSIGN(binary_writer); + DISALLOW_MOVE_AND_ASSIGN(binary_writer); }; //--------------- inline implementation ------------------- diff --git a/src/utils/ports.h b/src/utils/ports.h index 718810f052..7e76e2c4a0 100644 --- a/src/utils/ports.h +++ b/src/utils/ports.h @@ -63,9 +63,21 @@ #define dsn_likely(pred) (__builtin_expect((pred), 1)) #define dsn_unlikely(pred) (__builtin_expect((pred), 0)) -#define DISALLOW_COPY_AND_ASSIGN(TypeName) \ - TypeName(const TypeName &) = delete; \ - void operator=(const TypeName &) = delete +#define DECLARE_COPY_AND_ASSIGN(type, action) \ + type(const type &) = action; \ + type &operator=(const type &) = action + +#define DECLARE_MOVE_AND_ASSIGN(type, action) \ + type(type &&) = action; \ + type &operator=(type &&) = action + +#define DEFAULT_COPY_AND_ASSIGN(type) DECLARE_COPY_AND_ASSIGN(type, default) + +#define DEFAULT_MOVE_AND_ASSIGN(type) DECLARE_MOVE_AND_ASSIGN(type, default) + +#define DISALLOW_COPY_AND_ASSIGN(type) DECLARE_COPY_AND_ASSIGN(type, delete) + +#define DISALLOW_MOVE_AND_ASSIGN(type) DECLARE_MOVE_AND_ASSIGN(type, delete) #if defined OS_LINUX || defined OS_CYGWIN From 41abbbdeedf5d0c3987cbbd2eea5809ddd9e662c Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Fri, 8 Nov 2024 15:20:28 +0800 Subject: [PATCH 06/25] fix clang-tidy --- src/rpc/rpc_stream.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rpc/rpc_stream.h b/src/rpc/rpc_stream.h index 0d85960315..71624b130b 100644 --- a/src/rpc/rpc_stream.h +++ b/src/rpc/rpc_stream.h @@ -83,6 +83,8 @@ typedef ::dsn::ref_ptr rpc_read_stream_ptr; class rpc_write_stream : public binary_writer { public: + rpc_write_stream() = delete; + rpc_write_stream(message_ex *msg) : _msg(msg), _last_write_next_committed(true), _last_write_next_total_size(0) { @@ -136,10 +138,8 @@ class rpc_write_stream : public binary_writer message_ex *_msg; bool _last_write_next_committed; int _last_write_next_total_size; - - rpc_write_stream() = delete; }; -typedef ::dsn::ref_ptr rpc_write_stream_ptr; +using rpc_write_stream_ptr = dsn::ref_ptr; } // namespace dsn From 2c34c1c64d68760e2d0f6f318d90009307c3fa4b Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Fri, 8 Nov 2024 15:35:30 +0800 Subject: [PATCH 07/25] fix clang-tidy --- src/rpc/rpc_stream.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/rpc/rpc_stream.h b/src/rpc/rpc_stream.h index 71624b130b..ffaa049e2d 100644 --- a/src/rpc/rpc_stream.h +++ b/src/rpc/rpc_stream.h @@ -85,7 +85,7 @@ class rpc_write_stream : public binary_writer public: rpc_write_stream() = delete; - rpc_write_stream(message_ex *msg) + explicit rpc_write_stream(message_ex *msg) : _msg(msg), _last_write_next_committed(true), _last_write_next_total_size(0) { } @@ -138,6 +138,9 @@ class rpc_write_stream : public binary_writer message_ex *_msg; bool _last_write_next_committed; int _last_write_next_total_size; + + DISALLOW_COPY_AND_ASSIGN(rpc_write_stream); + DISALLOW_MOVE_AND_ASSIGN(rpc_write_stream); }; using rpc_write_stream_ptr = dsn::ref_ptr; From 1f948c752684aef01bdfd2dfdb41c06eee1386fd Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Sat, 9 Nov 2024 00:16:47 +0800 Subject: [PATCH 08/25] add server_state --- .../duplication/meta_duplication_service.cpp | 2 +- .../duplication/meta_duplication_service.h | 4 +- src/meta/server_state.cpp | 273 +++++++++++------- src/meta/server_state.h | 5 + src/utils/binary_writer.h | 1 + 5 files changed, 178 insertions(+), 107 deletions(-) diff --git a/src/meta/duplication/meta_duplication_service.cpp b/src/meta/duplication/meta_duplication_service.cpp index d0c87272d8..90123122f4 100644 --- a/src/meta/duplication/meta_duplication_service.cpp +++ b/src/meta/duplication/meta_duplication_service.cpp @@ -421,7 +421,7 @@ void meta_duplication_service::duplication_sync(duplication_sync_rpc rpc) if (dup->status() == duplication_status::DS_PREPARE) { create_follower_app_for_duplication(dup, app); } else if (dup->status() == duplication_status::DS_APP) { - check_follower_app_if_create_completed(dup); + mark_follower_app_created_for_duplication(dup, app); } } diff --git a/src/meta/duplication/meta_duplication_service.h b/src/meta/duplication/meta_duplication_service.h index 4f4b9af740..0dd53f5593 100644 --- a/src/meta/duplication/meta_duplication_service.h +++ b/src/meta/duplication/meta_duplication_service.h @@ -105,13 +105,13 @@ class meta_duplication_service void create_follower_app_for_duplication(const std::shared_ptr &dup, const std::shared_ptr &app); + void mark_follower_app_created_for_duplication(const std::shared_ptr &dup, + const std::shared_ptr &app); void do_create_follower_app_for_duplication( const std::shared_ptr &dup, const std::shared_ptr &app, const std::string &create_status, std::function create_callback); - void mark_follower_app_created_for_duplication(const std::shared_ptr &dup, - const std::shared_ptr &app); void on_follower_app_creating_for_duplication(const std::shared_ptr &dup, error_code err, configuration_create_app_response &&resp); diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index 064b570034..183818ee23 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -129,6 +129,14 @@ DSN_DECLARE_bool(recover_from_replica_server); namespace dsn { namespace replication { +#define REPLY_TO_CLIENT(msg, response) \ + _meta_svc->reply_data(msg, response); \ + msg->release_ref() + +#define REPLY_TO_CLIENT_AND_RETURN(msg, response) \ + REPLY_TO_CLIENT(msg, response); \ + return + static const char *lock_state = "lock"; static const char *unlock_state = "unlock"; @@ -207,6 +215,14 @@ int server_state::count_staging_app() return ans; } +#define INIT_CREATE_APP_RESPONSE_WITH_ERR(response, err_code) \ + configuration_create_app_response response; \ + response.err = err_code + +#define SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, err_code) \ + INIT_CREATE_APP_RESPONSE_WITH_ERR(response, err_code); \ + REPLY_TO_CLIENT_AND_RETURN(msg, response) + void server_state::transition_staging_state(std::shared_ptr &app) { #define send_response(meta, msg, response_data) \ @@ -221,8 +237,7 @@ void server_state::transition_staging_state(std::shared_ptr &app) app_status::type old_status = app->status; if (app->status == app_status::AS_CREATING) { app->status = app_status::AS_AVAILABLE; - configuration_create_app_response resp; - resp.err = dsn::ERR_OK; + INIT_CREATE_APP_RESPONSE_WITH_ERR(resp, dsn::ERR_OK); resp.appid = app->app_id; send_response(_meta_svc, app->helpers->pending_response, resp); } else if (app->status == app_status::AS_DROPPING) { @@ -1078,44 +1093,26 @@ void server_state::do_app_create(std::shared_ptr &app) app_dir, LPC_META_STATE_HIGH, on_create_app_root, value); } -namespace { - -bool is_follower_app_creating(const dsn::replication::app_state &app) -{ - const auto &iter = app.envs.find( - dsn::replication::duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); - if (iter == app.envs.end()) { - return false; - } - - return iter->second == dsn::replication::duplication_constants:: - kDuplicationEnvMasterCreateFollowerAppStatusCreating; -} - -} // anonymous namespace - void server_state::create_app(dsn::message_ex *msg) { configuration_create_app_request request; - configuration_create_app_response response; - std::shared_ptr app; - bool will_create_app = false; dsn::unmarshall(msg, request); - const auto &duplication_env_iterator = + const auto &master_cluster = request.options.envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); + bool duplicating = master_cluster != request.options.envs.end(); LOG_INFO("create app request, name({}), type({}), partition_count({}), replica_count({}), " "duplication({})", request.app_name, request.options.app_type, request.options.partition_count, request.options.replica_count, - duplication_env_iterator == request.options.envs.end() - ? "false" - : fmt::format( + duplicating + ? fmt::format( "{}.{}", request.options.envs[duplication_constants::kDuplicationEnvMasterClusterKey], - request.app_name)); + request.app_name) + : "false"); auto option_match_check = [](const create_app_options &opt, const app_state &exist_app) { return opt.partition_count == exist_app.partition_count && @@ -1127,71 +1124,136 @@ void server_state::create_app(dsn::message_ex *msg) auto level = _meta_svc->get_function_level(); if (level <= meta_function_level::fl_freezed) { LOG_ERROR("current meta function level is freezed, since there are too few alive nodes"); - response.err = ERR_STATE_FREEZED; - will_create_app = false; - } else if (request.options.partition_count <= 0 || - !validate_target_max_replica_count(request.options.replica_count)) { - response.err = ERR_INVALID_PARAMETERS; - will_create_app = false; - } else if (!_app_env_validator.validate_app_envs(request.options.envs)) { - response.err = ERR_INVALID_PARAMETERS; - will_create_app = false; - } else { - zauto_write_lock l(_lock); - app = get_app(request.app_name); - if (nullptr != app) { - switch (app->status) { - case app_status::AS_AVAILABLE: - if (!request.options.success_if_exist) { - response.err = is_follower_app_creating(*app) ? ERR_OK : ERR_APP_EXIST; - } else if (!option_match_check(request.options, *app)) { - response.err = ERR_INVALID_PARAMETERS; - } else { - response.err = ERR_OK; - response.appid = app->app_id; + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_STATE_FREEZED); + } + + if (request.options.partition_count <= 0 || + !validate_target_max_replica_count(request.options.replica_count)) { + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_PARAMETERS); + } + + if (!_app_env_validator.validate_app_envs(request.options.envs)) { + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_PARAMETERS); + } + + zauto_write_lock l(_lock); + + auto app = get_app(request.app_name); + if (nullptr != app) { + configuration_create_app_response response; + + switch (app->status) { + case app_status::AS_AVAILABLE: + if (!request.options.success_if_exist) { + if (duplicating) { + process_create_follower_app_status(msg, request, *app, master_cluster->second); + return; } - break; - case app_status::AS_CREATING: - case app_status::AS_RECALLING: - response.err = ERR_BUSY_CREATING; - break; - case app_status::AS_DROPPING: - response.err = ERR_BUSY_DROPPING; - break; - default: - break; + + response.err = ERR_APP_EXIST; + } else if (!option_match_check(request.options, *app)) { + response.err = ERR_INVALID_PARAMETERS; + } else { + response.err = ERR_OK; + response.appid = app->app_id; } - } else { - will_create_app = true; - - app_info info; - info.app_id = next_app_id(); - info.app_name = request.app_name; - info.app_type = request.options.app_type; - info.envs = std::move(request.options.envs); - info.is_stateful = request.options.is_stateful; - info.max_replica_count = request.options.replica_count; - info.partition_count = request.options.partition_count; - info.status = app_status::AS_CREATING; - info.create_second = dsn_now_ms() / 1000; - info.init_partition_count = request.options.partition_count; - - app = app_state::create(info); - app->helpers->pending_response = msg; - app->helpers->partitions_in_progress.store(info.partition_count); + break; + case app_status::AS_CREATING: + case app_status::AS_RECALLING: + response.err = ERR_BUSY_CREATING; + break; + case app_status::AS_DROPPING: + response.err = ERR_BUSY_DROPPING; + break; + default: + break; + } - _all_apps.emplace(app->app_id, app); - _exist_apps.emplace(request.app_name, app); - _table_metric_entities.create_entity(app->app_id, app->partition_count); + REPLY_TO_CLIENT_AND_RETURN(msg, response); + } + + app_info info; + info.app_id = next_app_id(); + info.app_name = request.app_name; + info.app_type = request.options.app_type; + info.envs = std::move(request.options.envs); + info.is_stateful = request.options.is_stateful; + info.max_replica_count = request.options.replica_count; + info.partition_count = request.options.partition_count; + info.status = app_status::AS_CREATING; + info.create_second = dsn_now_ms() / 1000; + info.init_partition_count = request.options.partition_count; + + app = app_state::create(info); + app->helpers->pending_response = msg; + app->helpers->partitions_in_progress.store(info.partition_count); + + _all_apps.emplace(app->app_id, app); + _exist_apps.emplace(request.app_name, app); + _table_metric_entities.create_entity(app->app_id, app->partition_count); + + do_app_create(app); +} + +void server_state::process_create_follower_app_status( + message_ex *msg, + const configuration_create_app_request &request, + const app_state &app, + const std::string &req_master_cluster) +{ + const auto &req_status = request.options.envs.find( + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); + if (req_status == request.options.envs.end()) { + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_APP_EXIST); + } + + const auto &my_status = + app.envs.find(duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); + if (my_status == app.envs.end()) { + return; + } + + if (my_status->second == + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating) { + if (req_status->second == + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating) { + const auto &my_master_cluster = + app.envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); + if (my_master_cluster == app.envs.end() || + my_master_cluster->second != req_master_cluster) { + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_APP_EXIST); + } + + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_OK); + } + + if (req_status->second == + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { + + return; } + + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_STATE); } - if (will_create_app) { - do_app_create(app); - } else { - _meta_svc->reply_data(msg, response); - msg->release_ref(); + if (my_status->second == + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { + if (req_status->second == + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating) { + + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_APP_EXIST); + } + + if (req_status->second == + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { + + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_OK); + } + + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_STATE); } + + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_STATE); } void server_state::do_app_drop(std::shared_ptr &app) @@ -1271,12 +1333,12 @@ void server_state::drop_app(dsn::message_ex *msg) } } } - if (do_dropping) { - do_app_drop(app); - } else { - _meta_svc->reply_data(msg, response); - msg->release_ref(); + + if (!do_dropping) { + REPLY_TO_CLIENT_AND_RETURN(msg, response); } + + do_app_drop(app); } void server_state::rename_app(configuration_rename_app_rpc rpc) @@ -1420,10 +1482,9 @@ void server_state::recall_app(dsn::message_ex *msg) } if (!do_recalling) { - _meta_svc->reply_data(msg, response); - msg->release_ref(); - return; + REPLY_TO_CLIENT_AND_RETURN(msg, response); } + do_app_recall(target_app); } @@ -1759,8 +1820,7 @@ void server_state::on_update_configuration_on_remote_reply( configuration_update_response resp; resp.err = ERR_OK; resp.config = config_request->config; - _meta_svc->reply_data(cc.msg, resp); - cc.msg->release_ref(); + REPLY_TO_CLIENT(cc.msg, resp); cc.msg = nullptr; } @@ -2047,17 +2107,16 @@ void server_state::on_update_configuration( } if (response.err != ERR_IO_PENDING) { - _meta_svc->reply_data(msg, response); - msg->release_ref(); - } else { - CHECK(config_status::not_pending == cc.stage, - "invalid config status, cc.stage = {}", - enum_to_string(cc.stage)); - cc.stage = config_status::pending_remote_sync; - cc.pending_sync_request = cfg_request; - cc.msg = msg; - cc.pending_sync_task = update_configuration_on_remote(cfg_request); + REPLY_TO_CLIENT_AND_RETURN(msg, response); } + + CHECK(config_status::not_pending == cc.stage, + "invalid config status, cc.stage = {}", + enum_to_string(cc.stage)); + cc.stage = config_status::pending_remote_sync; + cc.pending_sync_request = cfg_request; + cc.msg = msg; + cc.pending_sync_task = update_configuration_on_remote(cfg_request); } void server_state::on_partition_node_dead(std::shared_ptr &app, @@ -4071,5 +4130,11 @@ void server_state::recover_app_max_replica_count(std::shared_ptr &app &tracker); } +#undef SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN +#undef INIT_CREATE_APP_RESPONSE_WITH_ERR + +#undef REPLY_TO_CLIENT_AND_RETURN +#undef REPLY_TO_CLIENT + } // namespace replication } // namespace dsn diff --git a/src/meta/server_state.h b/src/meta/server_state.h index a787328f73..9135591a90 100644 --- a/src/meta/server_state.h +++ b/src/meta/server_state.h @@ -258,6 +258,11 @@ class server_state bool skip_lost_partitions, std::string &hint_message); + void process_create_follower_app_status(message_ex *msg, + const configuration_create_app_request &request, + const app_state &app, + const std::string &req_master_cluster); + void do_app_create(std::shared_ptr &app); void do_app_drop(std::shared_ptr &app); void do_app_recall(std::shared_ptr &app); diff --git a/src/utils/binary_writer.h b/src/utils/binary_writer.h index 816bec18ee..7bce6139db 100644 --- a/src/utils/binary_writer.h +++ b/src/utils/binary_writer.h @@ -34,6 +34,7 @@ #include #include "blob.h" +#include "utils/ports.h" namespace dsn { From 00f1009683009efda6f2c0143acd4ce97eafba92 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Sat, 9 Nov 2024 00:46:06 +0800 Subject: [PATCH 09/25] fix clang-tidy --- .clang-tidy | 2 +- build_tools/clang_tidy.py | 1 + src/meta/server_state.cpp | 8 +++----- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 888e91b95b..f6da755646 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -20,7 +20,7 @@ CheckOptions: [] # Disable some checks that are not useful for us now. # They are sorted by names, and should be consistent to build_tools/clang_tidy.py. -Checks: 'abseil-*,boost-*,bugprone-*,cert-*,clang-analyzer-*,concurrency-*,cppcoreguidelines-*,darwin-*,fuchsia-*,google-*,hicpp-*,linuxkernel-*,llvm-*,misc-*,modernize-*,performance-*,portability-*,readability-*,-bugprone-easily-swappable-parameters,-bugprone-lambda-function-name,-bugprone-macro-parentheses,-cert-err58-cpp,-concurrency-mt-unsafe,-cppcoreguidelines-avoid-c-arrays,-cppcoreguidelines-avoid-magic-numbers,-cppcoreguidelines-avoid-non-const-global-variables,-cppcoreguidelines-macro-usage,-cppcoreguidelines-non-private-member-variables-in-classes,-cppcoreguidelines-owning-memory,-cppcoreguidelines-pro-bounds-array-to-pointer-decay,-cppcoreguidelines-pro-bounds-pointer-arithmetic,-cppcoreguidelines-pro-type-const-cast,-cppcoreguidelines-pro-type-union-access,-fuchsia-default-arguments-calls,-fuchsia-overloaded-operator,-fuchsia-statically-constructed-objects,-google-readability-avoid-underscore-in-googletest-name,-hicpp-avoid-c-arrays,-hicpp-named-parameter,-hicpp-no-array-decay,-llvm-include-order,-misc-definitions-in-headers,-misc-non-private-member-variables-in-classes,-modernize-avoid-c-arrays,-modernize-replace-disallow-copy-and-assign-macro,-modernize-use-trailing-return-type,-readability-function-cognitive-complexity,-readability-identifier-length,-readability-magic-numbers,-readability-named-parameter' +Checks: 'abseil-*,boost-*,bugprone-*,cert-*,clang-analyzer-*,concurrency-*,cppcoreguidelines-*,darwin-*,fuchsia-*,google-*,hicpp-*,linuxkernel-*,llvm-*,misc-*,modernize-*,performance-*,portability-*,readability-*,-bugprone-easily-swappable-parameters,-bugprone-lambda-function-name,-bugprone-macro-parentheses,-cert-err58-cpp,-concurrency-mt-unsafe,-cppcoreguidelines-avoid-c-arrays,-cppcoreguidelines-avoid-magic-numbers,-cppcoreguidelines-avoid-non-const-global-variables,-cppcoreguidelines-macro-usage,-cppcoreguidelines-non-private-member-variables-in-classes,-cppcoreguidelines-owning-memory,-cppcoreguidelines-pro-bounds-array-to-pointer-decay,-cppcoreguidelines-pro-bounds-pointer-arithmetic,-cppcoreguidelines-pro-type-const-cast,-cppcoreguidelines-pro-type-union-access,-fuchsia-default-arguments-calls,-fuchsia-overloaded-operator,-fuchsia-statically-constructed-objects,-google-readability-avoid-underscore-in-googletest-name,-hicpp-avoid-c-arrays,-hicpp-named-parameter,-hicpp-no-array-decay,-llvm-include-order,-misc-definitions-in-headers,-misc-non-private-member-variables-in-classes,-modernize-avoid-c-arrays,-modernize-replace-disallow-copy-and-assign-macro,-modernize-use-trailing-return-type,-performance-unnecessary-value-param,-readability-function-cognitive-complexity,-readability-identifier-length,-readability-magic-numbers,-readability-named-parameter' ExtraArgs: ExtraArgsBefore: [] FormatStyle: none diff --git a/build_tools/clang_tidy.py b/build_tools/clang_tidy.py index b2ef72d9bb..e5ef014b88 100755 --- a/build_tools/clang_tidy.py +++ b/build_tools/clang_tidy.py @@ -90,6 +90,7 @@ def tidy_on_path(path): "-modernize-avoid-c-arrays," "-modernize-replace-disallow-copy-and-assign-macro," "-modernize-use-trailing-return-type," + "-performance-unnecessary-value-param," "-readability-function-cognitive-complexity," "-readability-identifier-length," "-readability-magic-numbers," diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index 183818ee23..da57d2f1d3 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -126,8 +126,7 @@ DSN_DEFINE_int32(meta_server, DSN_DECLARE_bool(recover_from_replica_server); -namespace dsn { -namespace replication { +namespace dsn::replication { #define REPLY_TO_CLIENT(msg, response) \ _meta_svc->reply_data(msg, response); \ @@ -1181,7 +1180,7 @@ void server_state::create_app(dsn::message_ex *msg) info.max_replica_count = request.options.replica_count; info.partition_count = request.options.partition_count; info.status = app_status::AS_CREATING; - info.create_second = dsn_now_ms() / 1000; + info.create_second = static_cast(dsn_now_s()); info.init_partition_count = request.options.partition_count; app = app_state::create(info); @@ -4136,5 +4135,4 @@ void server_state::recover_app_max_replica_count(std::shared_ptr &app #undef REPLY_TO_CLIENT_AND_RETURN #undef REPLY_TO_CLIENT -} // namespace replication -} // namespace dsn +} // namespace dsn::replication From 414c86e4ccbb9d8c366cbd0a504dd9dc7ac0f15d Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Mon, 11 Nov 2024 10:48:54 +0800 Subject: [PATCH 10/25] fix IWYU --- src/meta/server_state.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/meta/server_state.h b/src/meta/server_state.h index 9135591a90..511de09206 100644 --- a/src/meta/server_state.h +++ b/src/meta/server_state.h @@ -44,21 +44,22 @@ #include "dsn.layer2_types.h" #include "meta/meta_rpc_types.h" #include "meta_data.h" +#include "table_metrics.h" #include "task/task.h" #include "task/task_tracker.h" -#include "table_metrics.h" #include "utils/error_code.h" #include "utils/zlocks.h" namespace dsn { class blob; class command_deregister; -class message_ex; class host_port; +class message_ex; namespace replication { class configuration_balancer_request; class configuration_balancer_response; +class configuration_create_app_request; class configuration_list_apps_request; class configuration_list_apps_response; class configuration_proposal_action; From 0ff0330a62e7075b675621ec68d4891cd9ceb30d Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Mon, 11 Nov 2024 12:35:08 +0800 Subject: [PATCH 11/25] add --- src/meta/server_state.cpp | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index da57d2f1d3..a43ce92a96 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -1203,12 +1203,16 @@ void server_state::process_create_follower_app_status( const auto &req_status = request.options.envs.find( duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); if (req_status == request.options.envs.end()) { + // Still reply with ERR_APP_EXIST to the master cluster of old versions. SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_APP_EXIST); } const auto &my_status = app.envs.find(duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); if (my_status == app.envs.end()) { + // Since currently this table have been AS_AVAILABLE, it should have the env of + // creating status. + SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_STATE); return; } @@ -1220,38 +1224,48 @@ void server_state::process_create_follower_app_status( app.envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); if (my_master_cluster == app.envs.end() || my_master_cluster->second != req_master_cluster) { + // The source cluster is not matched. SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_APP_EXIST); } + // It is idempotent for the repeated requests. SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_OK); } if (req_status->second == duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { + // Update the creating status both on the remote storage and local memory. return; } + // Some undefined creating status from the source table. SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_STATE); } if (my_status->second == duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { - if (req_status->second == - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating) { - + const auto &my_master_cluster = + app.envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); + if (my_master_cluster == app.envs.end() || + my_master_cluster->second != req_master_cluster) { + // The source cluster is not matched. SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_APP_EXIST); } if (req_status->second == - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { - + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating || + req_status->second == + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { + // It is idempotent for the repeated requests. SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_OK); } + // Some undefined creating status from the source table. SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_STATE); } + // Some undefined creating status from the target table. SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_STATE); } From 133b7cc9ef2541394e42488adaaac6680c6dc4af Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Mon, 11 Nov 2024 15:21:46 +0800 Subject: [PATCH 12/25] add --- src/meta/server_state.cpp | 104 +++++++++++++++++++++++++++++--------- src/meta/server_state.h | 8 ++- 2 files changed, 86 insertions(+), 26 deletions(-) diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index a43ce92a96..d4801d21d0 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -218,10 +218,19 @@ int server_state::count_staging_app() configuration_create_app_response response; \ response.err = err_code -#define SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, err_code) \ +#define INIT_CREATE_APP_RESPONSE_WITH_OK(response, app_id) \ + configuration_create_app_response response; \ + response.err = dsn::ERR_OK; \ + response.appid = app_id; + +#define FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, err_code) \ INIT_CREATE_APP_RESPONSE_WITH_ERR(response, err_code); \ REPLY_TO_CLIENT_AND_RETURN(msg, response) +#define SUCC_CREATE_APP_RESPONSE_AND_RETURN(msg, response, app_id) \ + INIT_CREATE_APP_RESPONSE_WITH_OK(response, app_id); \ + REPLY_TO_CLIENT_AND_RETURN(msg, response) + void server_state::transition_staging_state(std::shared_ptr &app) { #define send_response(meta, msg, response_data) \ @@ -1123,16 +1132,16 @@ void server_state::create_app(dsn::message_ex *msg) auto level = _meta_svc->get_function_level(); if (level <= meta_function_level::fl_freezed) { LOG_ERROR("current meta function level is freezed, since there are too few alive nodes"); - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_STATE_FREEZED); + FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_STATE_FREEZED); } if (request.options.partition_count <= 0 || !validate_target_max_replica_count(request.options.replica_count)) { - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_PARAMETERS); + FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_PARAMETERS); } if (!_app_env_validator.validate_app_envs(request.options.envs)) { - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_PARAMETERS); + FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_PARAMETERS); } zauto_write_lock l(_lock); @@ -1145,7 +1154,7 @@ void server_state::create_app(dsn::message_ex *msg) case app_status::AS_AVAILABLE: if (!request.options.success_if_exist) { if (duplicating) { - process_create_follower_app_status(msg, request, *app, master_cluster->second); + process_create_follower_app_status(msg, request, master_cluster->second, app); return; } @@ -1197,22 +1206,22 @@ void server_state::create_app(dsn::message_ex *msg) void server_state::process_create_follower_app_status( message_ex *msg, const configuration_create_app_request &request, - const app_state &app, - const std::string &req_master_cluster) + const std::string &req_master_cluster, + std::shared_ptr &app) { const auto &req_status = request.options.envs.find( duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); if (req_status == request.options.envs.end()) { // Still reply with ERR_APP_EXIST to the master cluster of old versions. - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_APP_EXIST); + FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_APP_EXIST); } const auto &my_status = - app.envs.find(duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); - if (my_status == app.envs.end()) { + app->envs.find(duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); + if (my_status == app->envs.end()) { // Since currently this table have been AS_AVAILABLE, it should have the env of // creating status. - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_STATE); + FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_STATE); return; } @@ -1221,36 +1230,40 @@ void server_state::process_create_follower_app_status( if (req_status->second == duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating) { const auto &my_master_cluster = - app.envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); - if (my_master_cluster == app.envs.end() || + app->envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); + if (my_master_cluster == app->envs.end() || my_master_cluster->second != req_master_cluster) { // The source cluster is not matched. - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_APP_EXIST); + FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_APP_EXIST); } // It is idempotent for the repeated requests. - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_OK); + SUCC_CREATE_APP_RESPONSE_AND_RETURN(msg, response, app->app_id); } if (req_status->second == duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { // Update the creating status both on the remote storage and local memory. - + update_create_follower_app_status( + msg, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated, + app); return; } // Some undefined creating status from the source table. - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_STATE); + FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_STATE); } if (my_status->second == duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { const auto &my_master_cluster = - app.envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); - if (my_master_cluster == app.envs.end() || + app->envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); + if (my_master_cluster == app->envs.end() || my_master_cluster->second != req_master_cluster) { // The source cluster is not matched. - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_APP_EXIST); + FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_APP_EXIST); } if (req_status->second == @@ -1258,15 +1271,56 @@ void server_state::process_create_follower_app_status( req_status->second == duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { // It is idempotent for the repeated requests. - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_OK); + SUCC_CREATE_APP_RESPONSE_AND_RETURN(msg, response, app->app_id); } // Some undefined creating status from the source table. - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_STATE); + FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_STATE); } // Some undefined creating status from the target table. - SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN(msg, response, ERR_INVALID_STATE); + FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_STATE); +} + +void server_state::update_create_follower_app_status(message_ex *msg, + const std::string &old_status, + const std::string &new_status, + std::shared_ptr &app) +{ + auto ainfo = *(reinterpret_cast(app.get())); + ainfo.envs[duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey] = new_status; + auto app_path = get_app_path(*app); + do_update_app_info( + app_path, ainfo, [this, msg, old_status, new_status, app](error_code ec) mutable { + { + zauto_write_lock l(_lock); + + if (ec != ERR_OK) { + LOG_ERROR( + + "failed to update remote env of creating follower app status: " + "error_code={}, app_name={}, app_id={}, {}={} => {}", + ec, + app->app_name, + app->app_id, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + old_status, + new_status); + FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ec); + } + + app->envs[duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey] = + new_status; + LOG_INFO("both remote and local env of creating follower app status have been " + "updated successfully: app_name={}, app_id={}, {}={} => {}", + app->app_name, + app->app_id, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + old_status, + new_status); + SUCC_CREATE_APP_RESPONSE_AND_RETURN(msg, response, app->app_id); + } + }); } void server_state::do_app_drop(std::shared_ptr &app) @@ -4143,7 +4197,9 @@ void server_state::recover_app_max_replica_count(std::shared_ptr &app &tracker); } -#undef SEND_CREATE_APP_RESPONSE_TO_CLIENT_AND_RETURN +#undef SUCC_CREATE_APP_RESPONSE_AND_RETURN +#undef FAIL_CREATE_APP_RESPONSE_AND_RETURN +#undef INIT_CREATE_APP_RESPONSE_WITH_OK #undef INIT_CREATE_APP_RESPONSE_WITH_ERR #undef REPLY_TO_CLIENT_AND_RETURN diff --git a/src/meta/server_state.h b/src/meta/server_state.h index 511de09206..6e43c5ccaa 100644 --- a/src/meta/server_state.h +++ b/src/meta/server_state.h @@ -261,8 +261,12 @@ class server_state void process_create_follower_app_status(message_ex *msg, const configuration_create_app_request &request, - const app_state &app, - const std::string &req_master_cluster); + const std::string &req_master_cluster, + std::shared_ptr &app); + void update_create_follower_app_status(message_ex *msg, + const std::string &old_status, + const std::string &new_status, + std::shared_ptr &app); void do_app_create(std::shared_ptr &app); void do_app_drop(std::shared_ptr &app); From 95ac0518cec4322874e0a9c0a5cfdb1c64fecc46 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Mon, 11 Nov 2024 16:28:56 +0800 Subject: [PATCH 13/25] fix clang-tidy --- src/meta/server_state.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index d4801d21d0..2e47e96c53 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -1287,7 +1287,7 @@ void server_state::update_create_follower_app_status(message_ex *msg, const std::string &new_status, std::shared_ptr &app) { - auto ainfo = *(reinterpret_cast(app.get())); + app_info ainfo = *app; ainfo.envs[duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey] = new_status; auto app_path = get_app_path(*app); do_update_app_info( From 3b81283844ad1f45cb96d5ea273c283eadf4bab6 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Mon, 11 Nov 2024 18:42:49 +0800 Subject: [PATCH 14/25] add tests for meta_duplication_service --- .../duplication/meta_duplication_service.cpp | 46 ++++++++----- .../test/meta_duplication_service_test.cpp | 64 +++++++++++-------- 2 files changed, 66 insertions(+), 44 deletions(-) diff --git a/src/meta/duplication/meta_duplication_service.cpp b/src/meta/duplication/meta_duplication_service.cpp index 90123122f4..d7c3b6f176 100644 --- a/src/meta/duplication/meta_duplication_service.cpp +++ b/src/meta/duplication/meta_duplication_service.cpp @@ -545,14 +545,7 @@ void meta_duplication_service::on_follower_app_creating_for_duplication( FAIL_POINT_INJECT_F("persist_dup_status_failed", [](std::string_view) -> void { return; }); - if (update_err == ERR_OK) { - blob value = dup->to_json_blob(); - // Note: this function is `async`, it may not be persisted completed - // after executing, now using `_is_altering` to judge whether `updating` or - // `completed`, if `_is_altering`, dup->alter_status() will return `ERR_BUSY` - _meta_svc->get_meta_storage()->set_data( - std::string(dup->store_path), std::move(value), [dup]() { dup->persist_status(); }); - } else { + if (update_err != ERR_OK) { LOG_ERROR("create follower app[{}.{}] to trigger duplicate checkpoint failed: " "duplication_status = {}, create_err = {}, update_err = {}", dup->remote_cluster_name, @@ -560,7 +553,15 @@ void meta_duplication_service::on_follower_app_creating_for_duplication( duplication_status_to_string(dup->status()), create_err, update_err); + return; } + + blob value = dup->to_json_blob(); + // Note: this function is `async`, it may not be persisted completed + // after executing, now using `_is_altering` to judge whether `updating` or + // `completed`, if `_is_altering`, dup->alter_status() will return `ERR_BUSY` + _meta_svc->get_meta_storage()->set_data( + std::string(dup->store_path), std::move(value), [dup]() { dup->persist_status(); }); } void meta_duplication_service::on_follower_app_created_for_duplication( @@ -568,7 +569,18 @@ void meta_duplication_service::on_follower_app_created_for_duplication( error_code err, configuration_create_app_response &&resp) { + FAIL_POINT_INJECT_NOT_RETURN_F("on_follower_app_created", [&err](std::string_view s) -> void { + err = error_code(s.data()); + }); + if (err != ERR_OK || resp.err != ERR_OK) { + LOG_ERROR("mark follower app[{}.{}] created failed: " + "duplication_status = {}, err = {}, resp_err = {}", + dup->remote_cluster_name, + dup->remote_app_name, + duplication_status_to_string(dup->status()), + err, + resp.err); return; } @@ -697,15 +709,7 @@ void meta_duplication_service::check_follower_app_if_create_completed( FAIL_POINT_INJECT_F("persist_dup_status_failed", [](std::string_view) -> void { return; }); - if (update_err == ERR_OK) { - blob value = dup->to_json_blob(); - // Note: this function is `async`, it may not be persisted completed - // after executing, now using `_is_altering` to judge whether `updating` or - // `completed`, if `_is_altering`, dup->alter_status() will return `ERR_BUSY` - _meta_svc->get_meta_storage()->set_data(std::string(dup->store_path), - std::move(value), - [dup]() { dup->persist_status(); }); - } else { + if (update_err != ERR_OK) { LOG_ERROR("query follower app[{}.{}] replica configuration completed, result: " "duplication_status = {}, query_err = {}, update_err = {}", dup->remote_cluster_name, @@ -713,7 +717,15 @@ void meta_duplication_service::check_follower_app_if_create_completed( duplication_status_to_string(dup->status()), query_err, update_err); + return; } + + blob value = dup->to_json_blob(); + // Note: this function is `async`, it may not be persisted completed + // after executing, now using `_is_altering` to judge whether `updating` or + // `completed`, if `_is_altering`, dup->alter_status() will return `ERR_BUSY` + _meta_svc->get_meta_storage()->set_data( + std::string(dup->store_path), std::move(value), [dup]() { dup->persist_status(); }); }); } diff --git a/src/meta/test/meta_duplication_service_test.cpp b/src/meta/test/meta_duplication_service_test.cpp index af312fa9e7..06bd75c146 100644 --- a/src/meta/test/meta_duplication_service_test.cpp +++ b/src/meta/test/meta_duplication_service_test.cpp @@ -198,9 +198,10 @@ class meta_duplication_service_test : public meta_test_base dup_svc().create_follower_app_for_duplication(dup, app); } - void check_follower_app_if_create_completed(const std::shared_ptr &dup) + void mark_follower_app_created_for_duplication(const std::shared_ptr &dup, + const std::shared_ptr &app) { - dup_svc().check_follower_app_if_create_completed(dup); + dup_svc().mark_follower_app_created_for_duplication(dup, app); } duplication_status::type next_status(const std::shared_ptr &dup) const @@ -965,7 +966,7 @@ TEST_F(meta_duplication_service_test, create_follower_app_for_duplication) } } -TEST_F(meta_duplication_service_test, check_follower_app_if_create_completed) +TEST_F(meta_duplication_service_test, mark_follower_app_created_for_duplication) { struct test_case { @@ -977,59 +978,67 @@ TEST_F(meta_duplication_service_test, check_follower_app_if_create_completed) duplication_status::type next_status; } test_cases[] = {// 3 remote replicas with both primary and secondaries valid. {3, - {"create_app_ok"}, - {"void(true,2,0)"}, + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_OK)", "void(true,2,0)"}, false, duplication_status::DS_LOG, duplication_status::DS_INIT}, + // The follower app failed to be marked as created. + {3, + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_TIMEOUT)", "void(true,2,0)"}, + false, + duplication_status::DS_APP, + duplication_status::DS_INIT}, + // // 3 remote replicas with primary invalid and all secondaries valid. {3, - {"create_app_ok"}, - {"void(false,2,0)"}, + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_OK)", "void(false,2,0)"}, false, duplication_status::DS_APP, duplication_status::DS_INIT}, // 3 remote replicas with primary valid and only one secondary present // and valid. {3, - {"create_app_ok"}, - {"void(true,1,0)"}, + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_OK)", "void(true,1,0)"}, false, duplication_status::DS_LOG, duplication_status::DS_INIT}, // 3 remote replicas with primary valid and one secondary invalid. {3, - {"create_app_ok"}, - {"void(true,1,1)"}, + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_OK)", "void(true,1,1)"}, false, duplication_status::DS_APP, duplication_status::DS_INIT}, // 3 remote replicas with primary valid and only one secondary present // and invalid. {3, - {"create_app_ok"}, - {"void(true,0,1)"}, + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_OK)", "void(true,0,1)"}, false, duplication_status::DS_APP, duplication_status::DS_INIT}, // 3 remote replicas with primary valid and both secondaries absent. {3, - {"create_app_ok"}, - {"void(true,0,0)"}, + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_OK)", "void(true,0,0)"}, false, duplication_status::DS_APP, duplication_status::DS_INIT}, // 1 remote replicas with primary valid. {1, - {"create_app_ok"}, - {"void(true,0,0)"}, + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_OK)", "void(true,0,0)"}, false, duplication_status::DS_LOG, duplication_status::DS_INIT}, // 1 remote replicas with primary invalid. {1, - {"create_app_ok"}, - {"void(false,0,0)"}, + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_OK)", "void(false,0,0)"}, false, duplication_status::DS_APP, duplication_status::DS_INIT}, @@ -1037,36 +1046,37 @@ TEST_F(meta_duplication_service_test, check_follower_app_if_create_completed) // `check_follower_app_if_create_completed` would fail by default // in unit test. {3, - {"create_app_failed"}, - {"off()"}, + {"on_follower_app_created", "create_app_failed"}, + {"void(ERR_OK)", "off()"}, false, duplication_status::DS_APP, duplication_status::DS_INIT}, {3, - {"create_app_ok", "persist_dup_status_failed"}, - {"void(true,2,0)", "return()"}, + {"on_follower_app_created", "create_app_ok", "persist_dup_status_failed"}, + {"void(ERR_OK)", "void(true,2,0)", "return()"}, true, duplication_status::DS_APP, duplication_status::DS_LOG}}; size_t i = 0; for (const auto &test : test_cases) { - const auto &app_name = fmt::format("check_follower_app_if_create_completed_test_{}", i++); + const auto &app_name = + fmt::format("mark_follower_app_created_for_duplication_test_{}", i++); create_app(app_name); auto app = find_app(app_name); auto dup_add_resp = create_dup(app_name, test.remote_replica_count); auto dup = app->duplications[dup_add_resp.dupid]; - // 'check_follower_app_if_create_completed' must execute under duplication_status::DS_APP, - // so force update it. + // 'mark_follower_app_created_for_duplication' must execute under + // duplication_status::DS_APP, so force update it. force_update_dup_status(dup, duplication_status::DS_APP); fail::setup(); for (int i = 0; i < test.fail_cfg_name.size(); i++) { fail::cfg(test.fail_cfg_name[i], test.fail_cfg_action[i]); } - check_follower_app_if_create_completed(dup); + mark_follower_app_created_for_duplication(dup, app); wait_all(); fail::teardown(); From 9893e6480c215aa6dd2099b0ec4d5dd2de15e414 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Mon, 11 Nov 2024 19:50:57 +0800 Subject: [PATCH 15/25] fix clang-tidy --- .../test/meta_duplication_service_test.cpp | 113 ++++++++++-------- 1 file changed, 60 insertions(+), 53 deletions(-) diff --git a/src/meta/test/meta_duplication_service_test.cpp b/src/meta/test/meta_duplication_service_test.cpp index 06bd75c146..e059e18015 100644 --- a/src/meta/test/meta_duplication_service_test.cpp +++ b/src/meta/test/meta_duplication_service_test.cpp @@ -204,7 +204,8 @@ class meta_duplication_service_test : public meta_test_base dup_svc().mark_follower_app_created_for_duplication(dup, app); } - duplication_status::type next_status(const std::shared_ptr &dup) const + [[nodiscard]] static duplication_status::type + next_status(const std::shared_ptr &dup) { return dup->_next_status; } @@ -970,93 +971,99 @@ TEST_F(meta_duplication_service_test, mark_follower_app_created_for_duplication) { struct test_case { - int32_t remote_replica_count; std::vector fail_cfg_name; std::vector fail_cfg_action; - bool is_altering; + int32_t remote_replica_count; duplication_status::type cur_status; duplication_status::type next_status; + bool is_altering; } test_cases[] = {// 3 remote replicas with both primary and secondaries valid. - {3, - {"on_follower_app_created", "create_app_ok"}, + {{"on_follower_app_created", "create_app_ok"}, {"void(ERR_OK)", "void(true,2,0)"}, - false, + 3, duplication_status::DS_LOG, - duplication_status::DS_INIT}, + duplication_status::DS_INIT, + false}, // The follower app failed to be marked as created. - {3, - {"on_follower_app_created", "create_app_ok"}, + {{"on_follower_app_created", "create_app_ok"}, {"void(ERR_TIMEOUT)", "void(true,2,0)"}, - false, + 3, duplication_status::DS_APP, - duplication_status::DS_INIT}, + duplication_status::DS_INIT, + false}, // // 3 remote replicas with primary invalid and all secondaries valid. - {3, - {"on_follower_app_created", "create_app_ok"}, + {{"on_follower_app_created", "create_app_ok"}, {"void(ERR_OK)", "void(false,2,0)"}, - false, + 3, duplication_status::DS_APP, - duplication_status::DS_INIT}, + duplication_status::DS_INIT, + false}, // 3 remote replicas with primary valid and only one secondary present // and valid. - {3, - {"on_follower_app_created", "create_app_ok"}, + {{"on_follower_app_created", "create_app_ok"}, {"void(ERR_OK)", "void(true,1,0)"}, - false, + 3, duplication_status::DS_LOG, - duplication_status::DS_INIT}, + duplication_status::DS_INIT, + false}, // 3 remote replicas with primary valid and one secondary invalid. - {3, - {"on_follower_app_created", "create_app_ok"}, + {{"on_follower_app_created", "create_app_ok"}, {"void(ERR_OK)", "void(true,1,1)"}, - false, + 3, duplication_status::DS_APP, - duplication_status::DS_INIT}, + duplication_status::DS_INIT, + false}, // 3 remote replicas with primary valid and only one secondary present // and invalid. - {3, - {"on_follower_app_created", "create_app_ok"}, + {{"on_follower_app_created", "create_app_ok"}, {"void(ERR_OK)", "void(true,0,1)"}, - false, + 3, duplication_status::DS_APP, - duplication_status::DS_INIT}, + duplication_status::DS_INIT, + false}, // 3 remote replicas with primary valid and both secondaries absent. - {3, - {"on_follower_app_created", "create_app_ok"}, - {"void(ERR_OK)", "void(true,0,0)"}, - false, - duplication_status::DS_APP, - duplication_status::DS_INIT}, + { + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_OK)", "void(true,0,0)"}, + 3, + duplication_status::DS_APP, + duplication_status::DS_INIT, + false, + }, // 1 remote replicas with primary valid. - {1, - {"on_follower_app_created", "create_app_ok"}, - {"void(ERR_OK)", "void(true,0,0)"}, - false, - duplication_status::DS_LOG, - duplication_status::DS_INIT}, + { + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_OK)", "void(true,0,0)"}, + 1, + duplication_status::DS_LOG, + duplication_status::DS_INIT, + false, + }, // 1 remote replicas with primary invalid. - {1, - {"on_follower_app_created", "create_app_ok"}, - {"void(ERR_OK)", "void(false,0,0)"}, - false, - duplication_status::DS_APP, - duplication_status::DS_INIT}, + { + {"on_follower_app_created", "create_app_ok"}, + {"void(ERR_OK)", "void(false,0,0)"}, + 1, + duplication_status::DS_APP, + duplication_status::DS_INIT, + false, + }, // The case is just a "palace holder", actually // `check_follower_app_if_create_completed` would fail by default // in unit test. - {3, - {"on_follower_app_created", "create_app_failed"}, + {{"on_follower_app_created", "create_app_failed"}, {"void(ERR_OK)", "off()"}, - false, + 3, duplication_status::DS_APP, - duplication_status::DS_INIT}, - {3, - {"on_follower_app_created", "create_app_ok", "persist_dup_status_failed"}, + duplication_status::DS_INIT, + false}, + {{"on_follower_app_created", "create_app_ok", "persist_dup_status_failed"}, {"void(ERR_OK)", "void(true,2,0)", "return()"}, - true, + 3, duplication_status::DS_APP, - duplication_status::DS_LOG}}; + duplication_status::DS_LOG, + true}}; size_t i = 0; for (const auto &test : test_cases) { From 3cb2ec7085a861b8c0a2237395e9cbdf16cee8cd Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Tue, 12 Nov 2024 17:29:41 +0800 Subject: [PATCH 16/25] add meta_app_operation_test --- .../duplication/meta_duplication_service.cpp | 2 +- src/meta/test/meta_app_operation_test.cpp | 81 ++++++++++++++++--- .../test/meta_duplication_service_test.cpp | 2 +- 3 files changed, 74 insertions(+), 11 deletions(-) diff --git a/src/meta/duplication/meta_duplication_service.cpp b/src/meta/duplication/meta_duplication_service.cpp index d7c3b6f176..a48cc40155 100644 --- a/src/meta/duplication/meta_duplication_service.cpp +++ b/src/meta/duplication/meta_duplication_service.cpp @@ -25,7 +25,7 @@ #include #include -#include "common//duplication_common.h" +#include "common/duplication_common.h" #include "common/common.h" #include "common/gpid.h" #include "common/replication.codes.h" diff --git a/src/meta/test/meta_app_operation_test.cpp b/src/meta/test/meta_app_operation_test.cpp index f8b399d4c3..80e209ff89 100644 --- a/src/meta/test/meta_app_operation_test.cpp +++ b/src/meta/test/meta_app_operation_test.cpp @@ -26,6 +26,7 @@ #include #include +#include "common/duplication_common.h" #include "common/gpid.h" #include "common/json_helper.h" #include "common/replica_envs.h" @@ -62,7 +63,7 @@ namespace replication { class meta_app_operation_test : public meta_test_base { public: - meta_app_operation_test() {} + meta_app_operation_test() = default; error_code create_app_test(int32_t partition_count, int32_t replica_count, @@ -320,6 +321,34 @@ class meta_app_operation_test : public meta_test_base tracker.wait_outstanding_tasks(); } + void verify_app_envs(const std::string &app_name, + const std::map &expected_envs) + { + auto app = find_app(app_name); + CHECK(app, "app({}) does not exist", app_name); + + auto app_path = _ss->get_app_path(*app); + + dsn::task_tracker tracker; + _ms->get_remote_storage()->get_data( + app_path, + LPC_META_CALLBACK, + [app_name, expected_envs, app](error_code ec, const blob &value) { + ASSERT_EQ(ERR_OK, ec); + + app_info ainfo; + dsn::json::json_forwarder::decode(value, ainfo); + + ASSERT_EQ(app_name, app->app_name); + ASSERT_EQ(app_name, ainfo.app_name); + ASSERT_EQ(app->app_id, ainfo.app_id); + ASSERT_EQ(expected_envs, app->envs); + ASSERT_EQ(expected_envs, ainfo.envs); + }, + &tracker); + tracker.wait_outstanding_tasks(); + } + const std::string APP_NAME = "app_operation_test"; const std::string OLD_APP_NAME = "old_app_operation"; }; @@ -502,6 +531,34 @@ TEST_F(meta_app_operation_test, create_app) app_status::AS_INVALID, ERR_OK, {{"rocksdb.write_buffer_size", "33554432"}}}, + {APP_NAME + "_13", + 4, + 3, + 2, + 3, + 3, + false, + app_status::AS_INVALID, + ERR_OK, + {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kDuplicationEnvMasterAppNameKey, APP_NAME + "_13_remote"}, + {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, + {APP_NAME + "_13", + 4, + 3, + 2, + 3, + 3, + false, + app_status::AS_AVAILABLE, + ERR_OK, + {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kDuplicationEnvMasterAppNameKey, APP_NAME + "_13_remote"}, + {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, }; clear_nodes(); @@ -546,12 +603,17 @@ TEST_F(meta_app_operation_test, create_app) } else if (test.before_status != app_status::AS_INVALID) { update_app_status(test.before_status); } + auto err = create_app_test(test.partition_count, test.replica_count, test.success_if_exist, test.app_name, test.envs); - ASSERT_EQ(err, test.expected_err); + ASSERT_EQ(test.expected_err, err); + + if (test.expected_err == ERR_OK) { + verify_app_envs(test.app_name, test.envs); + } _ms->set_node_state(nodes, true); } @@ -570,38 +632,39 @@ TEST_F(meta_app_operation_test, create_app) ASSERT_TRUE(all_test_envs.find(option) != all_test_envs.end()); } } + // set FLAGS_min_allowed_replica_count successfully res = update_flag("min_allowed_replica_count", "2"); ASSERT_TRUE(res.is_ok()); - ASSERT_EQ(FLAGS_min_allowed_replica_count, 2); + ASSERT_EQ(2, FLAGS_min_allowed_replica_count); // set FLAGS_max_allowed_replica_count successfully res = update_flag("max_allowed_replica_count", "6"); ASSERT_TRUE(res.is_ok()); - ASSERT_EQ(FLAGS_max_allowed_replica_count, 6); + ASSERT_EQ(6, FLAGS_max_allowed_replica_count); // failed to set FLAGS_min_allowed_replica_count due to individual validation res = update_flag("min_allowed_replica_count", "0"); ASSERT_EQ(res.code(), ERR_INVALID_PARAMETERS); - ASSERT_EQ(FLAGS_min_allowed_replica_count, 2); + ASSERT_EQ(2, FLAGS_min_allowed_replica_count); std::cout << res.description() << std::endl; // failed to set FLAGS_max_allowed_replica_count due to individual validation res = update_flag("max_allowed_replica_count", "0"); ASSERT_EQ(res.code(), ERR_INVALID_PARAMETERS); - ASSERT_EQ(FLAGS_max_allowed_replica_count, 6); + ASSERT_EQ(6, FLAGS_max_allowed_replica_count); std::cout << res.description() << std::endl; // failed to set FLAGS_min_allowed_replica_count due to grouped validation res = update_flag("min_allowed_replica_count", "7"); ASSERT_EQ(res.code(), ERR_INVALID_PARAMETERS); - ASSERT_EQ(FLAGS_min_allowed_replica_count, 2); + ASSERT_EQ(2, FLAGS_min_allowed_replica_count); std::cout << res.description() << std::endl; // failed to set FLAGS_max_allowed_replica_count due to grouped validation res = update_flag("max_allowed_replica_count", "1"); ASSERT_EQ(res.code(), ERR_INVALID_PARAMETERS); - ASSERT_EQ(FLAGS_max_allowed_replica_count, 6); + ASSERT_EQ(6, FLAGS_max_allowed_replica_count); std::cout << res.description() << std::endl; // recover original FLAGS_min_allowed_replica_count @@ -614,7 +677,7 @@ TEST_F(meta_app_operation_test, create_app) res = update_flag("max_allowed_replica_count", std::to_string(reserved_max_allowed_replica_count)); ASSERT_TRUE(res.is_ok()); - ASSERT_EQ(FLAGS_max_allowed_replica_count, reserved_max_allowed_replica_count); + ASSERT_EQ(reserved_max_allowed_replica_count, FLAGS_max_allowed_replica_count); // recover original FLAGS_min_live_node_count_for_unfreeze set_min_live_node_count_for_unfreeze(reserved_min_live_node_count_for_unfreeze); diff --git a/src/meta/test/meta_duplication_service_test.cpp b/src/meta/test/meta_duplication_service_test.cpp index e059e18015..a8a6668f9c 100644 --- a/src/meta/test/meta_duplication_service_test.cpp +++ b/src/meta/test/meta_duplication_service_test.cpp @@ -76,7 +76,7 @@ class meta_duplication_service_test : public meta_test_base static const std::string kTestRemoteAppName; static const int32_t kTestRemoteReplicaCount; - meta_duplication_service_test() {} + meta_duplication_service_test() = default; duplication_add_response create_dup(const std::string &app_name, const std::string &remote_cluster, From acdfa0da2cc5f7d0cce288f24f4da50c202a2aab Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Tue, 12 Nov 2024 19:15:35 +0800 Subject: [PATCH 17/25] fix meta_app_operation_test --- src/meta/test/meta_app_operation_test.cpp | 95 ++++++++++++----------- 1 file changed, 50 insertions(+), 45 deletions(-) diff --git a/src/meta/test/meta_app_operation_test.cpp b/src/meta/test/meta_app_operation_test.cpp index 80e209ff89..9fae8df4c4 100644 --- a/src/meta/test/meta_app_operation_test.cpp +++ b/src/meta/test/meta_app_operation_test.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -356,51 +357,6 @@ class meta_app_operation_test : public meta_test_base TEST_F(meta_app_operation_test, create_app) { // Test cases: (assert min_allowed_replica_count <= max_allowed_replica_count) - // - wrong partition_count (< 0) - // - wrong partition_count (= 0) - // - wrong replica_count (< 0) - // - wrong replica_count (= 0) - // - wrong replica_count (> max_allowed_replica_count > alive_node_count) - // - wrong replica_count (> alive_node_count > max_allowed_replica_count) - // - wrong replica_count (> alive_node_count = max_allowed_replica_count) - // - wrong replica_count (= max_allowed_replica_count, and > alive_node_count) - // - wrong replica_count (< max_allowed_replica_count, and > alive_node_count) - // - wrong replica_count (= alive_node_count, and > max_allowed_replica_count) - // - wrong replica_count (< alive_node_count, and > max_allowed_replica_count) - // - valid replica_count (= max_allowed_replica_count, and = alive_node_count) - // - valid replica_count (= max_allowed_replica_count, and < alive_node_count) - // - valid replica_count (< max_allowed_replica_count, and = alive_node_count) - // - valid replica_count (< max_allowed_replica_count < alive_node_count) - // - valid replica_count (< alive_node_count < max_allowed_replica_count) - // - valid replica_count (< alive_node_count = max_allowed_replica_count) - // - wrong replica_count (< min_allowed_replica_count < alive_node_count) - // - wrong replica_count (< alive_node_count < min_allowed_replica_count) - // - wrong replica_count (< min_allowed_replica_count = alive_node_count) - // - wrong replica_count (< min_allowed_replica_count, and > alive_node_count) - // - wrong replica_count (< min_allowed_replica_count, and = alive_node_count) - // - wrong replica_count (= min_allowed_replica_count, and > alive_node_count) - // - valid replica_count (= min_allowed_replica_count, and < alive_node_count) - // - cluster freezed (alive_node_count = 0) - // - cluster freezed (alive_node_count = 1 < min_live_node_count_for_unfreeze) - // - cluster freezed (alive_node_count = 2 < min_live_node_count_for_unfreeze) - // - cluster not freezed (alive_node_count = min_live_node_count_for_unfreeze) - // - create succeed with single-replica - // - create succeed with double-replica - // - create app succeed - // - create failed with table existed - // - wrong app_status creating - // - wrong app_status recalling - // - wrong app_status dropping - // - create succeed with app_status dropped - // - create succeed with success_if_exist=true - // - wrong rocksdb.num_levels (< 1) - // - wrong rocksdb.num_levels (> 10) - // - wrong rocksdb.num_levels (non-digital character) - // - create app with rocksdb.num_levels (= 5) succeed - // - wrong rocksdb.write_buffer_size (< (16<<20)) - // - wrong rocksdb.write_buffer_size (> (512<<20)) - // - wrong rocksdb.write_buffer_size (non-digital character) - // - create app with rocksdb.write_buffer_size (= (32<<20)) succeed struct create_test { std::string app_name; @@ -414,43 +370,81 @@ TEST_F(meta_app_operation_test, create_app) error_code expected_err; std::map envs = {}; } tests[] = { + // Wrong partition_count (< 0). {APP_NAME, -1, 3, 2, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong partition_count (= 0). {APP_NAME, 0, 3, 2, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (< 0). {APP_NAME, 4, -1, 1, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (= 0). {APP_NAME, 4, 0, 1, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (> max_allowed_replica_count > alive_node_count). {APP_NAME, 4, 6, 2, 4, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (> alive_node_count > max_allowed_replica_count). {APP_NAME, 4, 7, 2, 6, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (> alive_node_count = max_allowed_replica_count). {APP_NAME, 4, 6, 2, 5, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (= max_allowed_replica_count, and > alive_node_count). {APP_NAME, 4, 5, 2, 4, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (< max_allowed_replica_count, and > alive_node_count). {APP_NAME, 4, 4, 2, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (= alive_node_count, and > max_allowed_replica_count). {APP_NAME, 4, 6, 2, 6, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (< alive_node_count, and > max_allowed_replica_count). {APP_NAME, 4, 6, 2, 7, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Valid replica_count (= max_allowed_replica_count, and = alive_node_count). {APP_NAME + "_1", 4, 5, 2, 5, 1, false, app_status::AS_INVALID, ERR_OK}, + // Valid replica_count (= max_allowed_replica_count, and < alive_node_count). {APP_NAME + "_2", 4, 5, 2, 6, 1, false, app_status::AS_INVALID, ERR_OK}, + // Valid replica_count (< max_allowed_replica_count, and = alive_node_count). {APP_NAME + "_3", 4, 4, 2, 4, 1, false, app_status::AS_INVALID, ERR_OK}, + // Valid replica_count (< max_allowed_replica_count < alive_node_count). {APP_NAME + "_4", 4, 4, 2, 6, 1, false, app_status::AS_INVALID, ERR_OK}, + // Valid replica_count (< alive_node_count < max_allowed_replica_count). {APP_NAME + "_5", 4, 3, 2, 4, 1, false, app_status::AS_INVALID, ERR_OK}, + // Valid replica_count (< alive_node_count = max_allowed_replica_count). {APP_NAME + "_6", 4, 4, 2, 5, 1, false, app_status::AS_INVALID, ERR_OK}, + // Wrong replica_count (< min_allowed_replica_count < alive_node_count). {APP_NAME, 4, 3, 2, 5, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (< alive_node_count < min_allowed_replica_count). {APP_NAME, 4, 3, 2, 4, 5, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (< min_allowed_replica_count = alive_node_count). {APP_NAME, 4, 3, 2, 4, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (< min_allowed_replica_count, and > alive_node_count). {APP_NAME, 4, 3, 2, 2, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (< min_allowed_replica_count, and = alive_node_count). {APP_NAME, 4, 3, 2, 3, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Wrong replica_count (= min_allowed_replica_count, and > alive_node_count). {APP_NAME, 4, 4, 2, 3, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS}, + // Valid replica_count (= min_allowed_replica_count, and < alive_node_count). {APP_NAME + "_7", 4, 3, 2, 4, 3, false, app_status::AS_INVALID, ERR_OK}, + // Cluster freezed (alive_node_count = 0). {APP_NAME, 4, 1, 1, 0, 1, false, app_status::AS_INVALID, ERR_STATE_FREEZED}, + // Cluster freezed (alive_node_count = 1 < min_live_node_count_for_unfreeze). {APP_NAME, 4, 2, 2, 1, 1, false, app_status::AS_INVALID, ERR_STATE_FREEZED}, + // Cluster freezed (alive_node_count = 2 < min_live_node_count_for_unfreeze). {APP_NAME, 4, 3, 3, 2, 1, false, app_status::AS_INVALID, ERR_STATE_FREEZED}, + // Cluster not freezed (alive_node_count = min_live_node_count_for_unfreeze). {APP_NAME + "_8", 4, 3, 3, 3, 1, false, app_status::AS_INVALID, ERR_OK}, + // Create succeed with single-replica. {APP_NAME + "_9", 4, 1, 1, 1, 1, false, app_status::AS_INVALID, ERR_OK}, + // Create succeed with double-replica. {APP_NAME + "_10", 4, 2, 1, 2, 2, false, app_status::AS_INVALID, ERR_OK}, + // Create app succeed. {APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_INVALID, ERR_OK}, + // Create failed with table existed. {APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_INVALID, ERR_APP_EXIST}, + // Wrong app_status creating. {APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_CREATING, ERR_BUSY_CREATING}, + // Wrong app_status recalling. {APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_RECALLING, ERR_BUSY_CREATING}, + // Wrong app_status dropping. {APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_DROPPING, ERR_BUSY_DROPPING}, + // Create succeed with app_status dropped. {APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_DROPPED, ERR_OK}, + // Create succeed with success_if_exist=true. {APP_NAME, 4, 3, 2, 3, 3, true, app_status::AS_INVALID, ERR_OK}, + // Wrong rocksdb.num_levels (< 1). {APP_NAME, 4, 3, @@ -461,6 +455,7 @@ TEST_F(meta_app_operation_test, create_app) app_status::AS_INVALID, ERR_INVALID_PARAMETERS, {{"rocksdb.num_levels", "0"}}}, + // Wrong rocksdb.num_levels (> 10). {APP_NAME, 4, 3, @@ -471,6 +466,7 @@ TEST_F(meta_app_operation_test, create_app) app_status::AS_INVALID, ERR_INVALID_PARAMETERS, {{"rocksdb.num_levels", "11"}}}, + // Wrong rocksdb.num_levels (non-digital character). {APP_NAME + "_11", 4, 3, @@ -481,6 +477,7 @@ TEST_F(meta_app_operation_test, create_app) app_status::AS_INVALID, ERR_INVALID_PARAMETERS, {{"rocksdb.num_levels", "5i"}}}, + // Create app with rocksdb.num_levels (= 5) succeed. {APP_NAME + "_11", 4, 3, @@ -491,6 +488,7 @@ TEST_F(meta_app_operation_test, create_app) app_status::AS_INVALID, ERR_OK, {{"rocksdb.num_levels", "5"}}}, + // Wrong rocksdb.write_buffer_size (< (16<<20)). {APP_NAME, 4, 3, @@ -501,6 +499,7 @@ TEST_F(meta_app_operation_test, create_app) app_status::AS_INVALID, ERR_INVALID_PARAMETERS, {{"rocksdb.write_buffer_size", "1000"}}}, + // Wrong rocksdb.write_buffer_size (> (512<<20)). {APP_NAME, 4, 3, @@ -511,6 +510,7 @@ TEST_F(meta_app_operation_test, create_app) app_status::AS_INVALID, ERR_INVALID_PARAMETERS, {{"rocksdb.write_buffer_size", "1073741824"}}}, + // Wrong rocksdb.write_buffer_size (non-digital character). {APP_NAME, 4, 3, @@ -521,6 +521,7 @@ TEST_F(meta_app_operation_test, create_app) app_status::AS_INVALID, ERR_INVALID_PARAMETERS, {{"rocksdb.write_buffer_size", "n33554432"}}}, + // Create app with rocksdb.write_buffer_size (= (32<<20)) succeed. {APP_NAME + "_12", 4, 3, @@ -531,6 +532,8 @@ TEST_F(meta_app_operation_test, create_app) app_status::AS_INVALID, ERR_OK, {{"rocksdb.write_buffer_size", "33554432"}}}, + // Process the first request of creating follower app for duplication from the + // source cluster. {APP_NAME + "_13", 4, 3, @@ -545,6 +548,8 @@ TEST_F(meta_app_operation_test, create_app) {duplication_constants::kDuplicationEnvMasterAppNameKey, APP_NAME + "_13_remote"}, {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, + // Process the repeated request of creating follower app for duplication from the + // source cluster. {APP_NAME + "_13", 4, 3, From e2cc783c81da3241ee653b9ea08f3c9b7f312ba6 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Tue, 12 Nov 2024 20:56:28 +0800 Subject: [PATCH 18/25] fix clang-tidy --- src/meta/test/meta_app_operation_test.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/meta/test/meta_app_operation_test.cpp b/src/meta/test/meta_app_operation_test.cpp index 9fae8df4c4..a40f330228 100644 --- a/src/meta/test/meta_app_operation_test.cpp +++ b/src/meta/test/meta_app_operation_test.cpp @@ -16,7 +16,7 @@ // under the License. #include -#include +#include #include #include #include @@ -568,26 +568,26 @@ TEST_F(meta_app_operation_test, create_app) clear_nodes(); - // keep the number of all nodes greater than that of alive nodes + // Keep the number of all nodes greater than that of alive nodes. const int total_node_count = 10; auto nodes = ensure_enough_alive_nodes(total_node_count); - // the meta function level will become freezed once + // The meta function level will become freezed once // alive_nodes * 100 < total_nodes * _node_live_percentage_threshold_for_update // even if alive_nodes >= min_live_node_count_for_unfreeze set_node_live_percentage_threshold_for_update(0); - // save original FLAGS_min_live_node_count_for_unfreeze + // Save original FLAGS_min_live_node_count_for_unfreeze auto reserved_min_live_node_count_for_unfreeze = FLAGS_min_live_node_count_for_unfreeze; - // save original FLAGS_max_allowed_replica_count + // Save original FLAGS_max_allowed_replica_count auto reserved_max_allowed_replica_count = FLAGS_max_allowed_replica_count; - // keep FLAGS_max_allowed_replica_count fixed in the tests + // Keep FLAGS_max_allowed_replica_count fixed in the tests auto res = update_flag("max_allowed_replica_count", "5"); ASSERT_TRUE(res.is_ok()); - // save original FLAGS_min_allowed_replica_count + // Save original FLAGS_min_allowed_replica_count. auto reserved_min_allowed_replica_count = FLAGS_min_allowed_replica_count; for (auto test : tests) { From bbcb2ed443518225d8a4fa98f2aa92ac7404add5 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Wed, 13 Nov 2024 18:02:41 +0800 Subject: [PATCH 19/25] add logs and tests --- src/meta/app_env_validator.cpp | 11 +- src/meta/server_state.cpp | 128 +++++++++++++++------- src/meta/test/meta_app_operation_test.cpp | 78 ++++++++++--- 3 files changed, 162 insertions(+), 55 deletions(-) diff --git a/src/meta/app_env_validator.cpp b/src/meta/app_env_validator.cpp index 06288cb611..1eeb46028d 100644 --- a/src/meta/app_env_validator.cpp +++ b/src/meta/app_env_validator.cpp @@ -26,6 +26,7 @@ #include #include +#include "common/duplication_common.h" #include "common/replica_envs.h" #include "http/http_status_code.h" #include "utils/fmt_logging.h" @@ -59,7 +60,7 @@ bool app_env_validator::validate_app_envs(const std::map= 1; }}}, {replica_envs::MANUAL_COMPACT_PERIODIC_BOTTOMMOST_LEVEL_COMPACTION, mcblc}, {replica_envs::REPLICA_ACCESS_CONTROLLER_ALLOWED_USERS, {ValueType::kString}}, - {replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES, {ValueType::kString}}}; + {replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES, {ValueType::kString}}, + {duplication_constants::kDuplicationEnvMasterClusterKey, {ValueType::kString}}, + {duplication_constants::kDuplicationEnvMasterMetasKey, {ValueType::kString}}, + {duplication_constants::kDuplicationEnvMasterAppNameKey, {ValueType::kString}}, + {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + {ValueType::kString}}, + }; } const std::unordered_map diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index 2e47e96c53..44dcaf0349 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -223,11 +223,11 @@ int server_state::count_staging_app() response.err = dsn::ERR_OK; \ response.appid = app_id; -#define FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, err_code) \ +#define FAIL_CREATE_APP_RESPONSE(msg, response, err_code) \ INIT_CREATE_APP_RESPONSE_WITH_ERR(response, err_code); \ REPLY_TO_CLIENT_AND_RETURN(msg, response) -#define SUCC_CREATE_APP_RESPONSE_AND_RETURN(msg, response, app_id) \ +#define SUCC_CREATE_APP_RESPONSE(msg, response, app_id) \ INIT_CREATE_APP_RESPONSE_WITH_OK(response, app_id); \ REPLY_TO_CLIENT_AND_RETURN(msg, response) @@ -1132,16 +1132,20 @@ void server_state::create_app(dsn::message_ex *msg) auto level = _meta_svc->get_function_level(); if (level <= meta_function_level::fl_freezed) { LOG_ERROR("current meta function level is freezed, since there are too few alive nodes"); - FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_STATE_FREEZED); + FAIL_CREATE_APP_RESPONSE(msg, response, ERR_STATE_FREEZED); } - if (request.options.partition_count <= 0 || - !validate_target_max_replica_count(request.options.replica_count)) { - FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_PARAMETERS); + if (request.options.partition_count <= 0) { + LOG_ERROR("partition_count({}) is invalid", request.options.partition_count); + FAIL_CREATE_APP_RESPONSE(msg, response, ERR_INVALID_PARAMETERS); + } + + if (!validate_target_max_replica_count(request.options.replica_count)) { + FAIL_CREATE_APP_RESPONSE(msg, response, ERR_INVALID_PARAMETERS); } if (!_app_env_validator.validate_app_envs(request.options.envs)) { - FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_PARAMETERS); + FAIL_CREATE_APP_RESPONSE(msg, response, ERR_INVALID_PARAMETERS); } zauto_write_lock l(_lock); @@ -1203,17 +1207,55 @@ void server_state::create_app(dsn::message_ex *msg) do_app_create(app); } +// It is idempotent for the repeated requests. +#define SUCC_IDEMPOTENT_CREATE_FOLLOWER_APP_STATUS() \ + LOG_INFO("repeated request that updates env {} of the follower app from {} to {}, " \ + "just ignore: app_name={}, app_id={}", \ + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, \ + my_status->second, \ + req_status->second, \ + app->app_name, \ + app->app_id); \ + SUCC_CREATE_APP_RESPONSE(msg, response, app->app_id) + +#define FAIL_UNDEFINED_CREATE_FOLLOWER_APP_STATUS(val, desc) \ + LOG_ERROR("undefined value({}) of env {} in the {}: app_name={}, app_id={}", \ + (val), \ + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, \ + (desc), \ + app->app_name, \ + app->app_id); \ + FAIL_CREATE_APP_RESPONSE(msg, response, ERR_INVALID_STATE) + void server_state::process_create_follower_app_status( message_ex *msg, const configuration_create_app_request &request, const std::string &req_master_cluster, std::shared_ptr &app) { + const auto &my_master_cluster = + app->envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); + if (my_master_cluster == app->envs.end() || my_master_cluster->second != req_master_cluster) { + // The source cluster is not matched. + LOG_ERROR("env {} are not matched between the request({}) and the follower " + "app({}): app_name={}, app_id={}", + duplication_constants::kDuplicationEnvMasterClusterKey, + req_master_cluster, + my_master_cluster == app->envs.end() ? "" : my_master_cluster->second, + app->app_name, + app->app_id); + FAIL_CREATE_APP_RESPONSE(msg, response, ERR_APP_EXIST); + } + const auto &req_status = request.options.envs.find( duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); if (req_status == request.options.envs.end()) { // Still reply with ERR_APP_EXIST to the master cluster of old versions. - FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_APP_EXIST); + LOG_ERROR("no env {} in the request: app_name={}, app_id={}", + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + app->app_name, + app->app_id); + FAIL_CREATE_APP_RESPONSE(msg, response, ERR_APP_EXIST); } const auto &my_status = @@ -1221,7 +1263,11 @@ void server_state::process_create_follower_app_status( if (my_status == app->envs.end()) { // Since currently this table have been AS_AVAILABLE, it should have the env of // creating status. - FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_STATE); + LOG_ERROR("no env {} in the follower app: app_name={}, app_id={}", + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + app->app_name, + app->app_id); + FAIL_CREATE_APP_RESPONSE(msg, response, ERR_INVALID_STATE); return; } @@ -1229,16 +1275,7 @@ void server_state::process_create_follower_app_status( duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating) { if (req_status->second == duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating) { - const auto &my_master_cluster = - app->envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); - if (my_master_cluster == app->envs.end() || - my_master_cluster->second != req_master_cluster) { - // The source cluster is not matched. - FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_APP_EXIST); - } - - // It is idempotent for the repeated requests. - SUCC_CREATE_APP_RESPONSE_AND_RETURN(msg, response, app->app_id); + SUCC_IDEMPOTENT_CREATE_FOLLOWER_APP_STATUS(); } if (req_status->second == @@ -1252,36 +1289,41 @@ void server_state::process_create_follower_app_status( return; } - // Some undefined creating status from the source table. - FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_STATE); + FAIL_UNDEFINED_CREATE_FOLLOWER_APP_STATUS(req_status->second, "request"); } if (my_status->second == duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { - const auto &my_master_cluster = - app->envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); - if (my_master_cluster == app->envs.end() || - my_master_cluster->second != req_master_cluster) { - // The source cluster is not matched. - FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_APP_EXIST); + if (req_status->second == + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating) { + // The status of the duplication should have been DS_APP since the follower app + // has been marked as created. Thus, the master cluster should never send the + // request with creating status again. + LOG_ERROR("the master cluster should never send the request with env {} valued {} " + "again since it has been {} in the follower app: app_name={}, app_id={}", + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + req_status->second, + my_status->second, + app->app_name, + app->app_id); + FAIL_CREATE_APP_RESPONSE(msg, response, ERR_INVALID_STATE); } if (req_status->second == - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating || - req_status->second == - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { - // It is idempotent for the repeated requests. - SUCC_CREATE_APP_RESPONSE_AND_RETURN(msg, response, app->app_id); + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { + SUCC_IDEMPOTENT_CREATE_FOLLOWER_APP_STATUS(); } - // Some undefined creating status from the source table. - FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_STATE); + FAIL_UNDEFINED_CREATE_FOLLOWER_APP_STATUS(req_status->second, "request"); } // Some undefined creating status from the target table. - FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ERR_INVALID_STATE); + FAIL_UNDEFINED_CREATE_FOLLOWER_APP_STATUS(my_status->second, "follower app"); } +#undef FAIL_UNDEFINED_CREATE_FOLLOWER_APP_STATUS +#undef SUCC_IDEMPOTENT_CREATE_FOLLOWER_APP_STATUS + void server_state::update_create_follower_app_status(message_ex *msg, const std::string &old_status, const std::string &new_status, @@ -1290,6 +1332,14 @@ void server_state::update_create_follower_app_status(message_ex *msg, app_info ainfo = *app; ainfo.envs[duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey] = new_status; auto app_path = get_app_path(*app); + + LOG_INFO("ready to update env {} of follower app from {} to {}, app_name={}, app_id={}, ", + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + old_status, + new_status, + app->app_name, + app->app_id); + do_update_app_info( app_path, ainfo, [this, msg, old_status, new_status, app](error_code ec) mutable { { @@ -1306,7 +1356,7 @@ void server_state::update_create_follower_app_status(message_ex *msg, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, old_status, new_status); - FAIL_CREATE_APP_RESPONSE_AND_RETURN(msg, response, ec); + FAIL_CREATE_APP_RESPONSE(msg, response, ec); } app->envs[duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey] = @@ -1318,7 +1368,7 @@ void server_state::update_create_follower_app_status(message_ex *msg, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, old_status, new_status); - SUCC_CREATE_APP_RESPONSE_AND_RETURN(msg, response, app->app_id); + SUCC_CREATE_APP_RESPONSE(msg, response, app->app_id); } }); } @@ -4197,8 +4247,8 @@ void server_state::recover_app_max_replica_count(std::shared_ptr &app &tracker); } -#undef SUCC_CREATE_APP_RESPONSE_AND_RETURN -#undef FAIL_CREATE_APP_RESPONSE_AND_RETURN +#undef SUCC_CREATE_APP_RESPONSE +#undef FAIL_CREATE_APP_RESPONSE #undef INIT_CREATE_APP_RESPONSE_WITH_OK #undef INIT_CREATE_APP_RESPONSE_WITH_ERR diff --git a/src/meta/test/meta_app_operation_test.cpp b/src/meta/test/meta_app_operation_test.cpp index a40f330228..4a027b4274 100644 --- a/src/meta/test/meta_app_operation_test.cpp +++ b/src/meta/test/meta_app_operation_test.cpp @@ -352,6 +352,8 @@ class meta_app_operation_test : public meta_test_base const std::string APP_NAME = "app_operation_test"; const std::string OLD_APP_NAME = "old_app_operation"; + const std::string DUP_MASTER_APP_NAME = "dup_master_test"; + const std::string DUP_FOLLOWER_APP_NAME = "dup_follower_test"; }; TEST_F(meta_app_operation_test, create_app) @@ -534,7 +536,7 @@ TEST_F(meta_app_operation_test, create_app) {{"rocksdb.write_buffer_size", "33554432"}}}, // Process the first request of creating follower app for duplication from the // source cluster. - {APP_NAME + "_13", + {DUP_FOLLOWER_APP_NAME, 4, 3, 2, @@ -545,12 +547,28 @@ TEST_F(meta_app_operation_test, create_app) ERR_OK, {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, - {duplication_constants::kDuplicationEnvMasterAppNameKey, APP_NAME + "_13_remote"}, + {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, + // Process the request of creating follower app for duplication from the wrong + // source cluster. + {DUP_FOLLOWER_APP_NAME, + 4, + 3, + 2, + 3, + 3, + false, + app_status::AS_AVAILABLE, + ERR_APP_EXIST, + {{duplication_constants::kDuplicationEnvMasterClusterKey, "another_source_cluster"}, + {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, // Process the repeated request of creating follower app for duplication from the // source cluster. - {APP_NAME + "_13", + {DUP_FOLLOWER_APP_NAME, 4, 3, 2, @@ -561,9 +579,41 @@ TEST_F(meta_app_operation_test, create_app) ERR_OK, {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, - {duplication_constants::kDuplicationEnvMasterAppNameKey, APP_NAME + "_13_remote"}, + {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, + // Process the request of marking follower app as created for duplication from the + // source cluster. + {DUP_FOLLOWER_APP_NAME, + 4, + 3, + 2, + 3, + 3, + false, + app_status::AS_AVAILABLE, + ERR_OK, + {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated}}}, + // Process the repeated request of marking follower app as created for duplication + // from the source cluster. + {DUP_FOLLOWER_APP_NAME, + 4, + 3, + 2, + 3, + 3, + false, + app_status::AS_AVAILABLE, + ERR_OK, + {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated}}}, }; clear_nodes(); @@ -590,7 +640,7 @@ TEST_F(meta_app_operation_test, create_app) // Save original FLAGS_min_allowed_replica_count. auto reserved_min_allowed_replica_count = FLAGS_min_allowed_replica_count; - for (auto test : tests) { + for (const auto &test : tests) { res = update_flag("min_allowed_replica_count", std::to_string(test.min_allowed_replica_count)); ASSERT_TRUE(res.is_ok()); @@ -638,53 +688,53 @@ TEST_F(meta_app_operation_test, create_app) } } - // set FLAGS_min_allowed_replica_count successfully + // Set FLAGS_min_allowed_replica_count successfully. res = update_flag("min_allowed_replica_count", "2"); ASSERT_TRUE(res.is_ok()); ASSERT_EQ(2, FLAGS_min_allowed_replica_count); - // set FLAGS_max_allowed_replica_count successfully + // Set FLAGS_max_allowed_replica_count successfully. res = update_flag("max_allowed_replica_count", "6"); ASSERT_TRUE(res.is_ok()); ASSERT_EQ(6, FLAGS_max_allowed_replica_count); - // failed to set FLAGS_min_allowed_replica_count due to individual validation + // Failed to set FLAGS_min_allowed_replica_count due to individual validation. res = update_flag("min_allowed_replica_count", "0"); ASSERT_EQ(res.code(), ERR_INVALID_PARAMETERS); ASSERT_EQ(2, FLAGS_min_allowed_replica_count); std::cout << res.description() << std::endl; - // failed to set FLAGS_max_allowed_replica_count due to individual validation + // Failed to set FLAGS_max_allowed_replica_count due to individual validation. res = update_flag("max_allowed_replica_count", "0"); ASSERT_EQ(res.code(), ERR_INVALID_PARAMETERS); ASSERT_EQ(6, FLAGS_max_allowed_replica_count); std::cout << res.description() << std::endl; - // failed to set FLAGS_min_allowed_replica_count due to grouped validation + // Failed to set FLAGS_min_allowed_replica_count due to grouped validation. res = update_flag("min_allowed_replica_count", "7"); ASSERT_EQ(res.code(), ERR_INVALID_PARAMETERS); ASSERT_EQ(2, FLAGS_min_allowed_replica_count); std::cout << res.description() << std::endl; - // failed to set FLAGS_max_allowed_replica_count due to grouped validation + // Failed to set FLAGS_max_allowed_replica_count due to grouped validation. res = update_flag("max_allowed_replica_count", "1"); ASSERT_EQ(res.code(), ERR_INVALID_PARAMETERS); ASSERT_EQ(6, FLAGS_max_allowed_replica_count); std::cout << res.description() << std::endl; - // recover original FLAGS_min_allowed_replica_count + // Recover original FLAGS_min_allowed_replica_count. res = update_flag("min_allowed_replica_count", std::to_string(reserved_min_allowed_replica_count)); ASSERT_TRUE(res.is_ok()); ASSERT_EQ(FLAGS_min_allowed_replica_count, reserved_min_allowed_replica_count); - // recover original FLAGS_max_allowed_replica_count + // Recover original FLAGS_max_allowed_replica_count. res = update_flag("max_allowed_replica_count", std::to_string(reserved_max_allowed_replica_count)); ASSERT_TRUE(res.is_ok()); ASSERT_EQ(reserved_max_allowed_replica_count, FLAGS_max_allowed_replica_count); - // recover original FLAGS_min_live_node_count_for_unfreeze + // Recover original FLAGS_min_live_node_count_for_unfreeze. set_min_live_node_count_for_unfreeze(reserved_min_live_node_count_for_unfreeze); } From 72b076e7c45cd85f66d04fce45f6c9fd785245a7 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Wed, 13 Nov 2024 19:15:33 +0800 Subject: [PATCH 20/25] add logs and tests --- .../duplication/meta_duplication_service.cpp | 18 ++++++- src/meta/server_state.cpp | 10 ++-- src/meta/test/meta_app_operation_test.cpp | 48 +++++++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/src/meta/duplication/meta_duplication_service.cpp b/src/meta/duplication/meta_duplication_service.cpp index a48cc40155..9ec49b9037 100644 --- a/src/meta/duplication/meta_duplication_service.cpp +++ b/src/meta/duplication/meta_duplication_service.cpp @@ -561,7 +561,14 @@ void meta_duplication_service::on_follower_app_creating_for_duplication( // after executing, now using `_is_altering` to judge whether `updating` or // `completed`, if `_is_altering`, dup->alter_status() will return `ERR_BUSY` _meta_svc->get_meta_storage()->set_data( - std::string(dup->store_path), std::move(value), [dup]() { dup->persist_status(); }); + std::string(dup->store_path), std::move(value), [dup]() { + dup->persist_status(); + LOG_INFO("create follower app[{}.{}] to trigger duplicate checkpoint successfully: " + "duplication_status = {}", + dup->remote_cluster_name, + dup->remote_app_name, + duplication_status_to_string(dup->status())); + }); } void meta_duplication_service::on_follower_app_created_for_duplication( @@ -725,7 +732,14 @@ void meta_duplication_service::check_follower_app_if_create_completed( // after executing, now using `_is_altering` to judge whether `updating` or // `completed`, if `_is_altering`, dup->alter_status() will return `ERR_BUSY` _meta_svc->get_meta_storage()->set_data( - std::string(dup->store_path), std::move(value), [dup]() { dup->persist_status(); }); + std::string(dup->store_path), std::move(value), [dup]() { + dup->persist_status(); + LOG_INFO("all of the replicas of follower app[{}.{}] have been ready: " + "duplication_status = {}", + dup->remote_cluster_name, + dup->remote_app_name, + duplication_status_to_string(dup->status())); + }); }); } diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index 44dcaf0349..ca0e364f48 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -1220,12 +1220,12 @@ void server_state::create_app(dsn::message_ex *msg) #define FAIL_UNDEFINED_CREATE_FOLLOWER_APP_STATUS(val, desc) \ LOG_ERROR("undefined value({}) of env {} in the {}: app_name={}, app_id={}", \ - (val), \ + val, \ duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, \ - (desc), \ + desc, \ app->app_name, \ app->app_id); \ - FAIL_CREATE_APP_RESPONSE(msg, response, ERR_INVALID_STATE) + FAIL_CREATE_APP_RESPONSE(msg, response, ERR_INVALID_PARAMETERS) void server_state::process_create_follower_app_status( message_ex *msg, @@ -1280,7 +1280,7 @@ void server_state::process_create_follower_app_status( if (req_status->second == duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { - // Update the creating status both on the remote storage and local memory. + // Mark the status as created both on the remote storage and local memory. update_create_follower_app_status( msg, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating, @@ -1306,7 +1306,7 @@ void server_state::process_create_follower_app_status( my_status->second, app->app_name, app->app_id); - FAIL_CREATE_APP_RESPONSE(msg, response, ERR_INVALID_STATE); + FAIL_CREATE_APP_RESPONSE(msg, response, ERR_APP_EXIST); } if (req_status->second == diff --git a/src/meta/test/meta_app_operation_test.cpp b/src/meta/test/meta_app_operation_test.cpp index 4a027b4274..88a7fb0ac9 100644 --- a/src/meta/test/meta_app_operation_test.cpp +++ b/src/meta/test/meta_app_operation_test.cpp @@ -566,6 +566,22 @@ TEST_F(meta_app_operation_test, create_app) {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, + // Process the request of creating follower app for duplication without the env of + // creating status. + {DUP_FOLLOWER_APP_NAME, + 4, + 3, + 2, + 3, + 3, + false, + app_status::AS_AVAILABLE, + ERR_APP_EXIST, + { + {duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + }}, // Process the repeated request of creating follower app for duplication from the // source cluster. {DUP_FOLLOWER_APP_NAME, @@ -598,6 +614,22 @@ TEST_F(meta_app_operation_test, create_app) {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated}}}, + // Process the request of creating follower app for duplication from the source + // cluster while it has been marked as created. + {DUP_FOLLOWER_APP_NAME, + 4, + 3, + 2, + 3, + 3, + false, + app_status::AS_AVAILABLE, + ERR_APP_EXIST, + {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, // Process the repeated request of marking follower app as created for duplication // from the source cluster. {DUP_FOLLOWER_APP_NAME, @@ -614,6 +646,22 @@ TEST_F(meta_app_operation_test, create_app) {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated}}}, + // Process the request of creating follower app for duplication with invalid creating + // status. + {DUP_FOLLOWER_APP_NAME, + 4, + 3, + 2, + 3, + 3, + false, + app_status::AS_AVAILABLE, + ERR_INVALID_PARAMETERS, + {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + "invalid_creating_status"}}}, }; clear_nodes(); From 868854624cf13da2449bb291ed793820facc9eee Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Thu, 14 Nov 2024 11:38:14 +0800 Subject: [PATCH 21/25] rename duplication env --- src/common/duplication_common.cpp | 17 ++--- src/common/duplication_common.h | 12 +-- src/meta/app_env_validator.cpp | 9 +-- .../duplication/meta_duplication_service.cpp | 18 ++--- src/meta/server_state.cpp | 69 +++++++---------- src/meta/test/meta_app_operation_test.cpp | 75 +++++++++---------- src/replica/duplication/replica_follower.cpp | 6 +- .../test/replica_follower_test.cpp | 33 ++++---- src/replica/replica_stub.cpp | 6 +- 9 files changed, 109 insertions(+), 136 deletions(-) diff --git a/src/common/duplication_common.cpp b/src/common/duplication_common.cpp index 7f44bf7412..cc49165f98 100644 --- a/src/common/duplication_common.cpp +++ b/src/common/duplication_common.cpp @@ -55,19 +55,16 @@ namespace replication { const std::string duplication_constants::kDuplicationCheckpointRootDir /*NOLINT*/ = "duplication"; const std::string duplication_constants::kClustersSectionName /*NOLINT*/ = "pegasus.clusters"; -const std::string duplication_constants::kDuplicationEnvMasterClusterKey /*NOLINT*/ = +const std::string duplication_constants::kEnvMasterClusterKey /*NOLINT*/ = "duplication.master_cluster"; -const std::string duplication_constants::kDuplicationEnvMasterMetasKey /*NOLINT*/ = - "duplication.master_metas"; -const std::string duplication_constants::kDuplicationEnvMasterAppNameKey /*NOLINT*/ = +const std::string duplication_constants::kEnvMasterMetasKey /*NOLINT*/ = "duplication.master_metas"; +const std::string duplication_constants::kEnvMasterAppNameKey /*NOLINT*/ = "duplication.master_app_name"; -const std::string duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey /*NOLINT*/ - = "duplication.master_create_follower_app_status"; -const std::string - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating /*NOLINT*/ +const std::string duplication_constants::kEnvFollowerAppStatusKey /*NOLINT*/ + = "duplication.follower_app_status"; +const std::string duplication_constants::kEnvFollowerAppStatusCreating /*NOLINT*/ = "creating"; -const std::string - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated /*NOLINT*/ +const std::string duplication_constants::kEnvFollowerAppStatusCreated /*NOLINT*/ = "created"; /*extern*/ const char *duplication_status_to_string(duplication_status::type status) diff --git a/src/common/duplication_common.h b/src/common/duplication_common.h index 84223c1f3d..0b42daef49 100644 --- a/src/common/duplication_common.h +++ b/src/common/duplication_common.h @@ -81,12 +81,12 @@ struct duplication_constants const static std::string kDuplicationCheckpointRootDir; const static std::string kClustersSectionName; // These will fill into app env and mark one app as a "follower app" and record master info - const static std::string kDuplicationEnvMasterClusterKey; - const static std::string kDuplicationEnvMasterMetasKey; - const static std::string kDuplicationEnvMasterAppNameKey; - const static std::string kDuplicationEnvMasterCreateFollowerAppStatusKey; - const static std::string kDuplicationEnvMasterCreateFollowerAppStatusCreating; - const static std::string kDuplicationEnvMasterCreateFollowerAppStatusCreated; + const static std::string kEnvMasterClusterKey; + const static std::string kEnvMasterMetasKey; + const static std::string kEnvMasterAppNameKey; + const static std::string kEnvFollowerAppStatusKey; + const static std::string kEnvFollowerAppStatusCreating; + const static std::string kEnvFollowerAppStatusCreated; }; USER_DEFINED_ENUM_FORMATTER(duplication_fail_mode::type) diff --git a/src/meta/app_env_validator.cpp b/src/meta/app_env_validator.cpp index 1eeb46028d..bb59b43e48 100644 --- a/src/meta/app_env_validator.cpp +++ b/src/meta/app_env_validator.cpp @@ -341,11 +341,10 @@ void app_env_validator::register_all_validators() {replica_envs::MANUAL_COMPACT_PERIODIC_BOTTOMMOST_LEVEL_COMPACTION, mcblc}, {replica_envs::REPLICA_ACCESS_CONTROLLER_ALLOWED_USERS, {ValueType::kString}}, {replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES, {ValueType::kString}}, - {duplication_constants::kDuplicationEnvMasterClusterKey, {ValueType::kString}}, - {duplication_constants::kDuplicationEnvMasterMetasKey, {ValueType::kString}}, - {duplication_constants::kDuplicationEnvMasterAppNameKey, {ValueType::kString}}, - {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, - {ValueType::kString}}, + {duplication_constants::kEnvMasterClusterKey, {ValueType::kString}}, + {duplication_constants::kEnvMasterMetasKey, {ValueType::kString}}, + {duplication_constants::kEnvMasterAppNameKey, {ValueType::kString}}, + {duplication_constants::kEnvFollowerAppStatusKey, {ValueType::kString}}, }; } diff --git a/src/meta/duplication/meta_duplication_service.cpp b/src/meta/duplication/meta_duplication_service.cpp index 9ec49b9037..a52161f941 100644 --- a/src/meta/duplication/meta_duplication_service.cpp +++ b/src/meta/duplication/meta_duplication_service.cpp @@ -467,7 +467,7 @@ void meta_duplication_service::create_follower_app_for_duplication( do_create_follower_app_for_duplication( dup, app, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating, + duplication_constants::kEnvFollowerAppStatusCreating, [this, dup](error_code err, configuration_create_app_response &&resp) { on_follower_app_creating_for_duplication(dup, err, std::move(resp)); }); @@ -479,7 +479,7 @@ void meta_duplication_service::mark_follower_app_created_for_duplication( do_create_follower_app_for_duplication( dup, app, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated, + duplication_constants::kEnvFollowerAppStatusCreated, [this, dup](error_code err, configuration_create_app_response &&resp) { on_follower_app_created_for_duplication(dup, err, std::move(resp)); }); @@ -502,16 +502,14 @@ void meta_duplication_service::do_create_follower_app_for_duplication( // add envs for follower table, which will use it know itself is `follower` and load master info // - env map: - // `kDuplicationEnvMasterClusterKey=>{master_cluster_name}` - // `kDuplicationEnvMasterMetasKey=>{master_meta_list}` - request.options.envs.emplace(duplication_constants::kDuplicationEnvMasterClusterKey, + // `kEnvMasterClusterKey=>{master_cluster_name}` + // `kEnvMasterMetasKey=>{master_meta_list}` + request.options.envs.emplace(duplication_constants::kEnvMasterClusterKey, get_current_dup_cluster_name()); - request.options.envs.emplace(duplication_constants::kDuplicationEnvMasterMetasKey, + request.options.envs.emplace(duplication_constants::kEnvMasterMetasKey, _meta_svc->get_meta_list_string()); - request.options.envs.emplace(duplication_constants::kDuplicationEnvMasterAppNameKey, - app->app_name); - request.options.envs.emplace( - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, create_status); + request.options.envs.emplace(duplication_constants::kEnvMasterAppNameKey, app->app_name); + request.options.envs.emplace(duplication_constants::kEnvFollowerAppStatusKey, create_status); host_port meta_servers; meta_servers.assign_group(dup->remote_cluster_name.c_str()); diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index ca0e364f48..fb041af6dc 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -1107,7 +1107,7 @@ void server_state::create_app(dsn::message_ex *msg) dsn::unmarshall(msg, request); const auto &master_cluster = - request.options.envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); + request.options.envs.find(duplication_constants::kEnvMasterClusterKey); bool duplicating = master_cluster != request.options.envs.end(); LOG_INFO("create app request, name({}), type({}), partition_count({}), replica_count({}), " "duplication({})", @@ -1116,10 +1116,9 @@ void server_state::create_app(dsn::message_ex *msg) request.options.partition_count, request.options.replica_count, duplicating - ? fmt::format( - "{}.{}", - request.options.envs[duplication_constants::kDuplicationEnvMasterClusterKey], - request.app_name) + ? fmt::format("{}.{}", + request.options.envs[duplication_constants::kEnvMasterClusterKey], + request.app_name) : "false"); auto option_match_check = [](const create_app_options &opt, const app_state &exist_app) { @@ -1211,7 +1210,7 @@ void server_state::create_app(dsn::message_ex *msg) #define SUCC_IDEMPOTENT_CREATE_FOLLOWER_APP_STATUS() \ LOG_INFO("repeated request that updates env {} of the follower app from {} to {}, " \ "just ignore: app_name={}, app_id={}", \ - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, \ + duplication_constants::kEnvFollowerAppStatusKey, \ my_status->second, \ req_status->second, \ app->app_name, \ @@ -1221,7 +1220,7 @@ void server_state::create_app(dsn::message_ex *msg) #define FAIL_UNDEFINED_CREATE_FOLLOWER_APP_STATUS(val, desc) \ LOG_ERROR("undefined value({}) of env {} in the {}: app_name={}, app_id={}", \ val, \ - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, \ + duplication_constants::kEnvFollowerAppStatusKey, \ desc, \ app->app_name, \ app->app_id); \ @@ -1233,13 +1232,12 @@ void server_state::process_create_follower_app_status( const std::string &req_master_cluster, std::shared_ptr &app) { - const auto &my_master_cluster = - app->envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); + const auto &my_master_cluster = app->envs.find(duplication_constants::kEnvMasterClusterKey); if (my_master_cluster == app->envs.end() || my_master_cluster->second != req_master_cluster) { // The source cluster is not matched. LOG_ERROR("env {} are not matched between the request({}) and the follower " "app({}): app_name={}, app_id={}", - duplication_constants::kDuplicationEnvMasterClusterKey, + duplication_constants::kEnvMasterClusterKey, req_master_cluster, my_master_cluster == app->envs.end() ? "" : my_master_cluster->second, app->app_name, @@ -1247,61 +1245,54 @@ void server_state::process_create_follower_app_status( FAIL_CREATE_APP_RESPONSE(msg, response, ERR_APP_EXIST); } - const auto &req_status = request.options.envs.find( - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); + const auto &req_status = + request.options.envs.find(duplication_constants::kEnvFollowerAppStatusKey); if (req_status == request.options.envs.end()) { // Still reply with ERR_APP_EXIST to the master cluster of old versions. LOG_ERROR("no env {} in the request: app_name={}, app_id={}", - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusKey, app->app_name, app->app_id); FAIL_CREATE_APP_RESPONSE(msg, response, ERR_APP_EXIST); } - const auto &my_status = - app->envs.find(duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey); + const auto &my_status = app->envs.find(duplication_constants::kEnvFollowerAppStatusKey); if (my_status == app->envs.end()) { // Since currently this table have been AS_AVAILABLE, it should have the env of // creating status. LOG_ERROR("no env {} in the follower app: app_name={}, app_id={}", - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusKey, app->app_name, app->app_id); FAIL_CREATE_APP_RESPONSE(msg, response, ERR_INVALID_STATE); return; } - if (my_status->second == - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating) { - if (req_status->second == - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating) { + if (my_status->second == duplication_constants::kEnvFollowerAppStatusCreating) { + if (req_status->second == duplication_constants::kEnvFollowerAppStatusCreating) { SUCC_IDEMPOTENT_CREATE_FOLLOWER_APP_STATUS(); } - if (req_status->second == - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { + if (req_status->second == duplication_constants::kEnvFollowerAppStatusCreated) { // Mark the status as created both on the remote storage and local memory. - update_create_follower_app_status( - msg, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated, - app); + update_create_follower_app_status(msg, + duplication_constants::kEnvFollowerAppStatusCreating, + duplication_constants::kEnvFollowerAppStatusCreated, + app); return; } FAIL_UNDEFINED_CREATE_FOLLOWER_APP_STATUS(req_status->second, "request"); } - if (my_status->second == - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { - if (req_status->second == - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating) { + if (my_status->second == duplication_constants::kEnvFollowerAppStatusCreated) { + if (req_status->second == duplication_constants::kEnvFollowerAppStatusCreating) { // The status of the duplication should have been DS_APP since the follower app // has been marked as created. Thus, the master cluster should never send the // request with creating status again. LOG_ERROR("the master cluster should never send the request with env {} valued {} " "again since it has been {} in the follower app: app_name={}, app_id={}", - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusKey, req_status->second, my_status->second, app->app_name, @@ -1309,8 +1300,7 @@ void server_state::process_create_follower_app_status( FAIL_CREATE_APP_RESPONSE(msg, response, ERR_APP_EXIST); } - if (req_status->second == - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated) { + if (req_status->second == duplication_constants::kEnvFollowerAppStatusCreated) { SUCC_IDEMPOTENT_CREATE_FOLLOWER_APP_STATUS(); } @@ -1330,11 +1320,11 @@ void server_state::update_create_follower_app_status(message_ex *msg, std::shared_ptr &app) { app_info ainfo = *app; - ainfo.envs[duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey] = new_status; + ainfo.envs[duplication_constants::kEnvFollowerAppStatusKey] = new_status; auto app_path = get_app_path(*app); LOG_INFO("ready to update env {} of follower app from {} to {}, app_name={}, app_id={}, ", - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusKey, old_status, new_status, app->app_name, @@ -1353,19 +1343,18 @@ void server_state::update_create_follower_app_status(message_ex *msg, ec, app->app_name, app->app_id, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusKey, old_status, new_status); FAIL_CREATE_APP_RESPONSE(msg, response, ec); } - app->envs[duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey] = - new_status; + app->envs[duplication_constants::kEnvFollowerAppStatusKey] = new_status; LOG_INFO("both remote and local env of creating follower app status have been " "updated successfully: app_name={}, app_id={}, {}={} => {}", app->app_name, app->app_id, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusKey, old_status, new_status); SUCC_CREATE_APP_RESPONSE(msg, response, app->app_id); diff --git a/src/meta/test/meta_app_operation_test.cpp b/src/meta/test/meta_app_operation_test.cpp index 88a7fb0ac9..9956ec31e2 100644 --- a/src/meta/test/meta_app_operation_test.cpp +++ b/src/meta/test/meta_app_operation_test.cpp @@ -545,11 +545,11 @@ TEST_F(meta_app_operation_test, create_app) false, app_status::AS_INVALID, ERR_OK, - {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, - {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, - {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, - {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, + {{duplication_constants::kEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kEnvFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusCreating}}}, // Process the request of creating follower app for duplication from the wrong // source cluster. {DUP_FOLLOWER_APP_NAME, @@ -561,11 +561,11 @@ TEST_F(meta_app_operation_test, create_app) false, app_status::AS_AVAILABLE, ERR_APP_EXIST, - {{duplication_constants::kDuplicationEnvMasterClusterKey, "another_source_cluster"}, - {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, - {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, - {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, + {{duplication_constants::kEnvMasterClusterKey, "another_source_cluster"}, + {duplication_constants::kEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kEnvFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusCreating}}}, // Process the request of creating follower app for duplication without the env of // creating status. {DUP_FOLLOWER_APP_NAME, @@ -578,9 +578,9 @@ TEST_F(meta_app_operation_test, create_app) app_status::AS_AVAILABLE, ERR_APP_EXIST, { - {duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, - {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, - {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, }}, // Process the repeated request of creating follower app for duplication from the // source cluster. @@ -593,11 +593,11 @@ TEST_F(meta_app_operation_test, create_app) false, app_status::AS_AVAILABLE, ERR_OK, - {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, - {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, - {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, - {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, + {{duplication_constants::kEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kEnvFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusCreating}}}, // Process the request of marking follower app as created for duplication from the // source cluster. {DUP_FOLLOWER_APP_NAME, @@ -609,11 +609,11 @@ TEST_F(meta_app_operation_test, create_app) false, app_status::AS_AVAILABLE, ERR_OK, - {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, - {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, - {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, - {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated}}}, + {{duplication_constants::kEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kEnvFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusCreated}}}, // Process the request of creating follower app for duplication from the source // cluster while it has been marked as created. {DUP_FOLLOWER_APP_NAME, @@ -625,11 +625,11 @@ TEST_F(meta_app_operation_test, create_app) false, app_status::AS_AVAILABLE, ERR_APP_EXIST, - {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, - {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, - {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, - {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreating}}}, + {{duplication_constants::kEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kEnvFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusCreating}}}, // Process the repeated request of marking follower app as created for duplication // from the source cluster. {DUP_FOLLOWER_APP_NAME, @@ -641,11 +641,11 @@ TEST_F(meta_app_operation_test, create_app) false, app_status::AS_AVAILABLE, ERR_OK, - {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, - {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, - {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, - {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, - duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusCreated}}}, + {{duplication_constants::kEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kEnvFollowerAppStatusKey, + duplication_constants::kEnvFollowerAppStatusCreated}}}, // Process the request of creating follower app for duplication with invalid creating // status. {DUP_FOLLOWER_APP_NAME, @@ -657,11 +657,10 @@ TEST_F(meta_app_operation_test, create_app) false, app_status::AS_AVAILABLE, ERR_INVALID_PARAMETERS, - {{duplication_constants::kDuplicationEnvMasterClusterKey, "source_cluster"}, - {duplication_constants::kDuplicationEnvMasterMetasKey, "10.1.2.3:34601"}, - {duplication_constants::kDuplicationEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, - {duplication_constants::kDuplicationEnvMasterCreateFollowerAppStatusKey, - "invalid_creating_status"}}}, + {{duplication_constants::kEnvMasterClusterKey, "source_cluster"}, + {duplication_constants::kEnvMasterMetasKey, "10.1.2.3:34601"}, + {duplication_constants::kEnvMasterAppNameKey, DUP_MASTER_APP_NAME}, + {duplication_constants::kEnvFollowerAppStatusKey, "invalid_creating_status"}}}, }; clear_nodes(); diff --git a/src/replica/duplication/replica_follower.cpp b/src/replica/duplication/replica_follower.cpp index 057358f59d..9aa5628b11 100644 --- a/src/replica/duplication/replica_follower.cpp +++ b/src/replica/duplication/replica_follower.cpp @@ -60,8 +60,8 @@ void replica_follower::init_master_info() { const auto &envs = _replica->get_app_info()->envs; - const auto &cluster_name = envs.find(duplication_constants::kDuplicationEnvMasterClusterKey); - const auto &metas = envs.find(duplication_constants::kDuplicationEnvMasterMetasKey); + const auto &cluster_name = envs.find(duplication_constants::kEnvMasterClusterKey); + const auto &metas = envs.find(duplication_constants::kEnvMasterMetasKey); if (cluster_name == envs.end() || metas == envs.end()) { return; } @@ -70,7 +70,7 @@ void replica_follower::init_master_info() _master_cluster_name = cluster_name->second; - const auto &app_name = envs.find(duplication_constants::kDuplicationEnvMasterAppNameKey); + const auto &app_name = envs.find(duplication_constants::kEnvMasterAppNameKey); if (app_name == envs.end()) { // The version of meta server of master cluster is old(< v2.6.0), thus the app name of // the follower cluster is the same with master cluster. diff --git a/src/replica/duplication/test/replica_follower_test.cpp b/src/replica/duplication/test/replica_follower_test.cpp index 45cf7e9dfa..cc25eb841b 100644 --- a/src/replica/duplication/test/replica_follower_test.cpp +++ b/src/replica/duplication/test/replica_follower_test.cpp @@ -62,9 +62,8 @@ class replica_follower_test : public duplication_test_base void update_mock_replica(const dsn::app_info &app) { bool is_duplication_follower = - (app.envs.find(duplication_constants::kDuplicationEnvMasterClusterKey) != - app.envs.end()) && - (app.envs.find(duplication_constants::kDuplicationEnvMasterMetasKey) != app.envs.end()); + (app.envs.find(duplication_constants::kEnvMasterClusterKey) != app.envs.end()) && + (app.envs.find(duplication_constants::kEnvMasterMetasKey) != app.envs.end()); _mock_replica = stub->generate_replica_ptr( app, gpid(2, 1), partition_status::PS_PRIMARY, 1, false, is_duplication_follower); } @@ -115,9 +114,8 @@ class replica_follower_test : public duplication_test_base void test_init_master_info(const std::string &expected_master_app_name) { - _app_info.envs.emplace(duplication_constants::kDuplicationEnvMasterClusterKey, - kTestMasterClusterName); - _app_info.envs.emplace(duplication_constants::kDuplicationEnvMasterMetasKey, + _app_info.envs.emplace(duplication_constants::kEnvMasterClusterKey, kTestMasterClusterName); + _app_info.envs.emplace(duplication_constants::kEnvMasterMetasKey, "127.0.0.1:34801,127.0.0.1:34802,127.0.0.1:34803"); update_mock_replica(_app_info); @@ -156,16 +154,14 @@ TEST_P(replica_follower_test, test_init_master_info_without_master_app_env) TEST_P(replica_follower_test, test_init_master_info_with_master_app_env) { static const std::string kTestAnotherMasterAppName("another_follower"); - _app_info.envs.emplace(duplication_constants::kDuplicationEnvMasterAppNameKey, - kTestAnotherMasterAppName); + _app_info.envs.emplace(duplication_constants::kEnvMasterAppNameKey, kTestAnotherMasterAppName); test_init_master_info(kTestAnotherMasterAppName); } TEST_P(replica_follower_test, test_duplicate_checkpoint) { - _app_info.envs.emplace(duplication_constants::kDuplicationEnvMasterClusterKey, - kTestMasterClusterName); - _app_info.envs.emplace(duplication_constants::kDuplicationEnvMasterMetasKey, + _app_info.envs.emplace(duplication_constants::kEnvMasterClusterKey, kTestMasterClusterName); + _app_info.envs.emplace(duplication_constants::kEnvMasterMetasKey, "127.0.0.1:34801,127.0.0.1:34802,127.0.0.1:34803"); update_mock_replica(_app_info); @@ -184,9 +180,8 @@ TEST_P(replica_follower_test, test_duplicate_checkpoint) TEST_P(replica_follower_test, test_async_duplicate_checkpoint_from_master_replica) { - _app_info.envs.emplace(duplication_constants::kDuplicationEnvMasterClusterKey, - kTestMasterClusterName); - _app_info.envs.emplace(duplication_constants::kDuplicationEnvMasterMetasKey, + _app_info.envs.emplace(duplication_constants::kEnvMasterClusterKey, kTestMasterClusterName); + _app_info.envs.emplace(duplication_constants::kEnvMasterMetasKey, "127.0.0.1:34801,127.0.0.1:34802,127.0.0.1:34803"); update_mock_replica(_app_info); @@ -207,9 +202,8 @@ TEST_P(replica_follower_test, test_async_duplicate_checkpoint_from_master_replic TEST_P(replica_follower_test, test_update_master_replica_config) { - _app_info.envs.emplace(duplication_constants::kDuplicationEnvMasterClusterKey, - kTestMasterClusterName); - _app_info.envs.emplace(duplication_constants::kDuplicationEnvMasterMetasKey, + _app_info.envs.emplace(duplication_constants::kEnvMasterClusterKey, kTestMasterClusterName); + _app_info.envs.emplace(duplication_constants::kEnvMasterMetasKey, "127.0.0.1:34801,127.0.0.1:34802,127.0.0.1:34803"); update_mock_replica(_app_info); auto follower = _mock_replica->get_replica_follower(); @@ -265,9 +259,8 @@ TEST_P(replica_follower_test, test_update_master_replica_config) TEST_P(replica_follower_test, test_nfs_copy_checkpoint) { - _app_info.envs.emplace(duplication_constants::kDuplicationEnvMasterClusterKey, - kTestMasterClusterName); - _app_info.envs.emplace(duplication_constants::kDuplicationEnvMasterMetasKey, + _app_info.envs.emplace(duplication_constants::kEnvMasterClusterKey, kTestMasterClusterName); + _app_info.envs.emplace(duplication_constants::kEnvMasterMetasKey, "127.0.0.1:34801,127.0.0.1:34802,127.0.0.1:34803"); update_mock_replica(_app_info); init_nfs(); diff --git a/src/replica/replica_stub.cpp b/src/replica/replica_stub.cpp index de2b789730..6389ed50c0 100644 --- a/src/replica/replica_stub.cpp +++ b/src/replica/replica_stub.cpp @@ -1844,10 +1844,8 @@ void replica_stub::open_replica( bool is_duplication_follower = ((configuration_update != nullptr) && (configuration_update->type == config_type::CT_ASSIGN_PRIMARY) && - (app.envs.find(duplication_constants::kDuplicationEnvMasterClusterKey) != - app.envs.end()) && - (app.envs.find(duplication_constants::kDuplicationEnvMasterMetasKey) != - app.envs.end())); + (app.envs.find(duplication_constants::kEnvMasterClusterKey) != app.envs.end()) && + (app.envs.find(duplication_constants::kEnvMasterMetasKey) != app.envs.end())); // NOTICE: when we don't need execute restore-process, we should remove a.b.pegasus // directory because it don't contain the valid data dir and also we need create a new From 39f4875f10d03519c53a9436f27022c6b52410b9 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Thu, 14 Nov 2024 15:45:11 +0800 Subject: [PATCH 22/25] add comments and fix clang-tidy --- .../duplication/meta_duplication_service.cpp | 20 +++++++++++++++++++ .../duplication/meta_duplication_service.h | 15 ++++++++++++++ .../test/meta_state/meta_state_service.cpp | 3 ++- .../test/replica_follower_test.cpp | 2 +- 4 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/meta/duplication/meta_duplication_service.cpp b/src/meta/duplication/meta_duplication_service.cpp index a52161f941..456ee21ae6 100644 --- a/src/meta/duplication/meta_duplication_service.cpp +++ b/src/meta/duplication/meta_duplication_service.cpp @@ -464,6 +464,19 @@ void meta_duplication_service::duplication_sync(duplication_sync_rpc rpc) void meta_duplication_service::create_follower_app_for_duplication( const std::shared_ptr &dup, const std::shared_ptr &app) { + // The request of creating table might be issued to the follower cluster many times, + // since some error might occurred while the follower table is being created. For + // example, once the follower cluster could no connect to the master cluster, the + // checkpoint would not be duplicated to the master cluster then the follower table + // failed to be created; in this case, the master cluster would repeat this request + // until the follower is created. + // + // To make this request idempotent, it would carry a status marking the follower as + // creating by an environment variable. The follower cluster would also store the + // status with the table as an env. As long as the status of the table is creating, + // the follower cluster would accept the request even if it is repeated (actually + // just ignore) and reply with ok. On the side of master cluster, it would send the + // same request periodically as long as it is still at the status of DS_PREPARE. do_create_follower_app_for_duplication( dup, app, @@ -476,6 +489,13 @@ void meta_duplication_service::create_follower_app_for_duplication( void meta_duplication_service::mark_follower_app_created_for_duplication( const std::shared_ptr &dup, const std::shared_ptr &app) { + // Once the status of duplication has become DS_APP, the master cluster would send + // another request to the follower cluster to mark the table as created. If the + // table is at the status of creating, the follower cluster should accept the request + // and update its status as created. Similarly, the master cluster could send this + // request repeatedly and the follower would accept. The follower would reject if it + // receives some invalid request(for example, a request that tries to create the same + // table again. do_create_follower_app_for_duplication( dup, app, diff --git a/src/meta/duplication/meta_duplication_service.h b/src/meta/duplication/meta_duplication_service.h index 0dd53f5593..1cb700d7da 100644 --- a/src/meta/duplication/meta_duplication_service.h +++ b/src/meta/duplication/meta_duplication_service.h @@ -103,22 +103,37 @@ class meta_duplication_service int32_t partition_idx, const duplication_confirm_entry &confirm_entry); + // Send a request to the follower cluster to create a table for duplication. This request + // is idempotent and is allowed to be sent multiple times as long as the duplication is at + // the status of DS_PREPARE. void create_follower_app_for_duplication(const std::shared_ptr &dup, const std::shared_ptr &app); + + // Send a request to the follower cluster to mark a table as created for duplication. This + // request is idempotent and is allowed to be sent multiple times as long as the duplication + // is at the status of DS_APP. void mark_follower_app_created_for_duplication(const std::shared_ptr &dup, const std::shared_ptr &app); + + // Send a request to the follower cluster to create a table or mark it as some specific + // status. void do_create_follower_app_for_duplication( const std::shared_ptr &dup, const std::shared_ptr &app, const std::string &create_status, std::function create_callback); + + // Callback for the response of creaing a new table. void on_follower_app_creating_for_duplication(const std::shared_ptr &dup, error_code err, configuration_create_app_response &&resp); + + // Callback for the response of marking a table as created. void on_follower_app_created_for_duplication(const std::shared_ptr &dup, error_code err, configuration_create_app_response &&resp); + // Check if the whole follower table(including all of its partitions and replicas) is ready. void check_follower_app_if_create_completed(const std::shared_ptr &dup); // Get zk path for duplication. diff --git a/src/meta/test/meta_state/meta_state_service.cpp b/src/meta/test/meta_state/meta_state_service.cpp index ae0d1f3f43..6ca22721de 100644 --- a/src/meta/test/meta_state/meta_state_service.cpp +++ b/src/meta/test/meta_state/meta_state_service.cpp @@ -99,7 +99,7 @@ void provider_basic_test(const service_creator_func &service_creator, service->delete_node("/1", false, META_STATE_SERVICE_SIMPLE_TEST_CALLBACK, expect_ok) ->wait(); } - // set & get data + // create & get data { dsn::binary_writer writer; writer.write(0xdeadbeef); @@ -119,6 +119,7 @@ void provider_basic_test(const service_creator_func &service_creator, }) ->wait(); } + // set & get data { dsn::binary_writer writer; writer.write(0xbeefdead); diff --git a/src/replica/duplication/test/replica_follower_test.cpp b/src/replica/duplication/test/replica_follower_test.cpp index cc25eb841b..c47c90a31f 100644 --- a/src/replica/duplication/test/replica_follower_test.cpp +++ b/src/replica/duplication/test/replica_follower_test.cpp @@ -206,7 +206,7 @@ TEST_P(replica_follower_test, test_update_master_replica_config) _app_info.envs.emplace(duplication_constants::kEnvMasterMetasKey, "127.0.0.1:34801,127.0.0.1:34802,127.0.0.1:34803"); update_mock_replica(_app_info); - auto follower = _mock_replica->get_replica_follower(); + auto *follower = _mock_replica->get_replica_follower(); query_cfg_response resp; ASSERT_EQ(update_master_replica_config(follower, resp), ERR_INCONSISTENT_STATE); From 81625b6aaf890afcf965bcf82f68d0f99e63b8df Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Thu, 14 Nov 2024 17:16:42 +0800 Subject: [PATCH 23/25] add comments --- src/meta/server_state.cpp | 29 +++++++++++++++++++---------- src/meta/server_state.h | 6 ++++++ 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index fb041af6dc..8d7c97f2f1 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -128,10 +128,12 @@ DSN_DECLARE_bool(recover_from_replica_server); namespace dsn::replication { +// Reply to the client with specified response. #define REPLY_TO_CLIENT(msg, response) \ _meta_svc->reply_data(msg, response); \ msg->release_ref() +// Reply to the client with specified response, and return from current function. #define REPLY_TO_CLIENT_AND_RETURN(msg, response) \ REPLY_TO_CLIENT(msg, response); \ return @@ -214,19 +216,27 @@ int server_state::count_staging_app() return ans; } +// Create a new variable of `configuration_create_app_response` and assign it with specified +// error code. #define INIT_CREATE_APP_RESPONSE_WITH_ERR(response, err_code) \ configuration_create_app_response response; \ response.err = err_code +// Create a new variable of `configuration_create_app_response` and assign it with ERR_OK +// and table id. #define INIT_CREATE_APP_RESPONSE_WITH_OK(response, app_id) \ configuration_create_app_response response; \ response.err = dsn::ERR_OK; \ response.appid = app_id; +// Reply to the client with a newly created failed `configuration_create_app_response` and return +// from current function. #define FAIL_CREATE_APP_RESPONSE(msg, response, err_code) \ INIT_CREATE_APP_RESPONSE_WITH_ERR(response, err_code); \ REPLY_TO_CLIENT_AND_RETURN(msg, response) +// Reply to the client with a newly created successful `configuration_create_app_response` and +// return from current function. #define SUCC_CREATE_APP_RESPONSE(msg, response, app_id) \ INIT_CREATE_APP_RESPONSE_WITH_OK(response, app_id); \ REPLY_TO_CLIENT_AND_RETURN(msg, response) @@ -1217,6 +1227,7 @@ void server_state::create_app(dsn::message_ex *msg) app->app_id); \ SUCC_CREATE_APP_RESPONSE(msg, response, app->app_id) +// Failed due to invalid creating status. #define FAIL_UNDEFINED_CREATE_FOLLOWER_APP_STATUS(val, desc) \ LOG_ERROR("undefined value({}) of env {} in the {}: app_name={}, app_id={}", \ val, \ @@ -1336,16 +1347,14 @@ void server_state::update_create_follower_app_status(message_ex *msg, zauto_write_lock l(_lock); if (ec != ERR_OK) { - LOG_ERROR( - - "failed to update remote env of creating follower app status: " - "error_code={}, app_name={}, app_id={}, {}={} => {}", - ec, - app->app_name, - app->app_id, - duplication_constants::kEnvFollowerAppStatusKey, - old_status, - new_status); + LOG_ERROR("failed to update remote env of creating follower app status: " + "error_code={}, app_name={}, app_id={}, {}={} => {}", + ec, + app->app_name, + app->app_id, + duplication_constants::kEnvFollowerAppStatusKey, + old_status, + new_status); FAIL_CREATE_APP_RESPONSE(msg, response, ec); } diff --git a/src/meta/server_state.h b/src/meta/server_state.h index 6e43c5ccaa..ea9a418bad 100644 --- a/src/meta/server_state.h +++ b/src/meta/server_state.h @@ -259,10 +259,16 @@ class server_state bool skip_lost_partitions, std::string &hint_message); + // Process the status carried in the environment variables of creating table request while + // the table is at the status of AS_AVAILABLE, to update remote and local states and reply + // to the master cluster. void process_create_follower_app_status(message_ex *msg, const configuration_create_app_request &request, const std::string &req_master_cluster, std::shared_ptr &app); + + // Update the meta data with the new creating status both on the remote storage and local + // memory. void update_create_follower_app_status(message_ex *msg, const std::string &old_status, const std::string &new_status, From b648a1609768d76fe650d9367610e679c69119e1 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Thu, 14 Nov 2024 19:04:21 +0800 Subject: [PATCH 24/25] add logs --- .../duplication/meta_duplication_service.cpp | 38 ++++++++++++++----- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/src/meta/duplication/meta_duplication_service.cpp b/src/meta/duplication/meta_duplication_service.cpp index 456ee21ae6..b0858e9341 100644 --- a/src/meta/duplication/meta_duplication_service.cpp +++ b/src/meta/duplication/meta_duplication_service.cpp @@ -537,6 +537,15 @@ void meta_duplication_service::do_create_follower_app_for_duplication( dsn::message_ex *msg = dsn::message_ex::create_request(RPC_CM_CREATE_APP); dsn::marshall(msg, request); + + LOG_INFO("send request to create follower app(cluster_name={}, app_name={}, status={}) " + "to trigger duplicate checkpoint: master_app_name={}, duplication_status={}", + dup->remote_cluster_name, + dup->remote_app_name, + create_status, + app->app_name, + duplication_status_to_string(dup->status())); + rpc::call(dsn::dns_resolver::instance().resolve_address(meta_servers), msg, _meta_svc->tracker(), @@ -564,8 +573,8 @@ void meta_duplication_service::on_follower_app_creating_for_duplication( FAIL_POINT_INJECT_F("persist_dup_status_failed", [](std::string_view) -> void { return; }); if (update_err != ERR_OK) { - LOG_ERROR("create follower app[{}.{}] to trigger duplicate checkpoint failed: " - "duplication_status = {}, create_err = {}, update_err = {}", + LOG_ERROR("create follower app(cluster_name={}, app_name={}) to trigger duplicate " + "checkpoint failed: duplication_status={}, create_err={}, update_err={}", dup->remote_cluster_name, dup->remote_app_name, duplication_status_to_string(dup->status()), @@ -581,8 +590,8 @@ void meta_duplication_service::on_follower_app_creating_for_duplication( _meta_svc->get_meta_storage()->set_data( std::string(dup->store_path), std::move(value), [dup]() { dup->persist_status(); - LOG_INFO("create follower app[{}.{}] to trigger duplicate checkpoint successfully: " - "duplication_status = {}", + LOG_INFO("create follower app(cluster_name={}, app_name={}) to trigger duplicate " + "checkpoint successfully: duplication_status={}", dup->remote_cluster_name, dup->remote_app_name, duplication_status_to_string(dup->status())); @@ -599,8 +608,8 @@ void meta_duplication_service::on_follower_app_created_for_duplication( }); if (err != ERR_OK || resp.err != ERR_OK) { - LOG_ERROR("mark follower app[{}.{}] created failed: " - "duplication_status = {}, err = {}, resp_err = {}", + LOG_ERROR("mark follower app(cluster_name={}, app_name={}) as created failed: " + "duplication_status={}, callback_err={}, resp_err={}", dup->remote_cluster_name, dup->remote_app_name, duplication_status_to_string(dup->status()), @@ -680,6 +689,14 @@ void meta_duplication_service::check_follower_app_if_create_completed( dsn::message_ex *msg = dsn::message_ex::create_request(RPC_CM_QUERY_PARTITION_CONFIG_BY_INDEX); dsn::marshall(msg, meta_config_request); + + LOG_INFO("send request to check if all replicas of follower app(cluster_name={}, " + "app_name={}) are ready: master_app_name={}, duplication_status={}", + dup->remote_cluster_name, + dup->remote_app_name, + dup->app_name, + duplication_status_to_string(dup->status())); + rpc::call( dsn::dns_resolver::instance().resolve_address(meta_servers), msg, @@ -735,8 +752,9 @@ void meta_duplication_service::check_follower_app_if_create_completed( [](std::string_view) -> void { return; }); if (update_err != ERR_OK) { - LOG_ERROR("query follower app[{}.{}] replica configuration completed, result: " - "duplication_status = {}, query_err = {}, update_err = {}", + LOG_ERROR("query follower app(cluster_name={}, app_name={}) replica " + "configuration completed, result: duplication_status={}, " + "query_err={}, update_err={}", dup->remote_cluster_name, dup->remote_app_name, duplication_status_to_string(dup->status()), @@ -752,8 +770,8 @@ void meta_duplication_service::check_follower_app_if_create_completed( _meta_svc->get_meta_storage()->set_data( std::string(dup->store_path), std::move(value), [dup]() { dup->persist_status(); - LOG_INFO("all of the replicas of follower app[{}.{}] have been ready: " - "duplication_status = {}", + LOG_INFO("all replicas of follower app(cluster_name={}, app_name={}) " + "have been ready: duplication_status={}", dup->remote_cluster_name, dup->remote_app_name, duplication_status_to_string(dup->status())); From 39ac18dfc9ac6deb9bfb251b882c13a618d12ddb Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Thu, 14 Nov 2024 19:25:24 +0800 Subject: [PATCH 25/25] fix logs --- .../duplication/meta_duplication_service.cpp | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/meta/duplication/meta_duplication_service.cpp b/src/meta/duplication/meta_duplication_service.cpp index b0858e9341..c557c890a2 100644 --- a/src/meta/duplication/meta_duplication_service.cpp +++ b/src/meta/duplication/meta_duplication_service.cpp @@ -574,9 +574,11 @@ void meta_duplication_service::on_follower_app_creating_for_duplication( if (update_err != ERR_OK) { LOG_ERROR("create follower app(cluster_name={}, app_name={}) to trigger duplicate " - "checkpoint failed: duplication_status={}, create_err={}, update_err={}", + "checkpoint failed: master_app_name={}, duplication_status={}, " + "create_err={}, update_err={}", dup->remote_cluster_name, dup->remote_app_name, + dup->app_name, duplication_status_to_string(dup->status()), create_err, update_err); @@ -591,9 +593,10 @@ void meta_duplication_service::on_follower_app_creating_for_duplication( std::string(dup->store_path), std::move(value), [dup]() { dup->persist_status(); LOG_INFO("create follower app(cluster_name={}, app_name={}) to trigger duplicate " - "checkpoint successfully: duplication_status={}", + "checkpoint successfully: master_app_name={}, duplication_status={}", dup->remote_cluster_name, dup->remote_app_name, + dup->app_name, duplication_status_to_string(dup->status())); }); } @@ -609,9 +612,10 @@ void meta_duplication_service::on_follower_app_created_for_duplication( if (err != ERR_OK || resp.err != ERR_OK) { LOG_ERROR("mark follower app(cluster_name={}, app_name={}) as created failed: " - "duplication_status={}, callback_err={}, resp_err={}", + "master_app_name={}, duplication_status={}, callback_err={}, resp_err={}", dup->remote_cluster_name, dup->remote_app_name, + dup->app_name, duplication_status_to_string(dup->status()), err, resp.err); @@ -690,7 +694,7 @@ void meta_duplication_service::check_follower_app_if_create_completed( dsn::message_ex *msg = dsn::message_ex::create_request(RPC_CM_QUERY_PARTITION_CONFIG_BY_INDEX); dsn::marshall(msg, meta_config_request); - LOG_INFO("send request to check if all replicas of follower app(cluster_name={}, " + LOG_INFO("send request to check if all replicas of creating follower app(cluster_name={}, " "app_name={}) are ready: master_app_name={}, duplication_status={}", dup->remote_cluster_name, dup->remote_app_name, @@ -752,11 +756,12 @@ void meta_duplication_service::check_follower_app_if_create_completed( [](std::string_view) -> void { return; }); if (update_err != ERR_OK) { - LOG_ERROR("query follower app(cluster_name={}, app_name={}) replica " - "configuration completed, result: duplication_status={}, " + LOG_ERROR("check all replicas of creating follower app(cluster_name={}, " + "app_name={}): master_app_name={}, duplication_status={}, " "query_err={}, update_err={}", dup->remote_cluster_name, dup->remote_app_name, + dup->app_name, duplication_status_to_string(dup->status()), query_err, update_err); @@ -771,9 +776,10 @@ void meta_duplication_service::check_follower_app_if_create_completed( std::string(dup->store_path), std::move(value), [dup]() { dup->persist_status(); LOG_INFO("all replicas of follower app(cluster_name={}, app_name={}) " - "have been ready: duplication_status={}", + "have been ready: master_app_name={}, duplication_status={}", dup->remote_cluster_name, dup->remote_app_name, + dup->app_name, duplication_status_to_string(dup->status())); }); });