From 2a8df3c42f41f4c917077aaf23037453674b52cb Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Thu, 21 Nov 2024 11:48:46 +0800 Subject: [PATCH] fix tests --- src/meta/server_state.cpp | 27 ++++++++++------- src/meta/test/server_state_test.cpp | 46 ++++++++++++++++++----------- 2 files changed, 44 insertions(+), 29 deletions(-) diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index 8e690d2d0b..ae0ecc1b93 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -1006,7 +1006,7 @@ void server_state::query_configuration_by_index(const query_cfg_request &request /*out*/ query_cfg_response &response) { zauto_read_lock l(_lock); - auto iter = _exist_apps.find(request.app_name.c_str()); + auto iter = _exist_apps.find(request.app_name); if (iter == _exist_apps.end()) { response.err = ERR_OBJECT_NOT_FOUND; return; @@ -3024,22 +3024,26 @@ void server_state::set_app_envs(const app_env_rpc &env_rpc) app_info ainfo; std::string app_path; { - zauto_read_lock l(_lock); - - auto app = get_app(app_name); + FAIL_POINT_INJECT_NOT_RETURN_F("set_app_envs_failed", [app_name, this](std::string_view s) { + zauto_write_lock l(_lock); - FAIL_POINT_INJECT_NOT_RETURN_F("set_app_envs_failed", [&app](std::string_view s) { if (s == "not_found") { - app.reset(); + CHECK_EQ(_exist_apps.erase(app_name), 1); return; } if (s == "dropping") { - app->status = app_status::AS_DROPPING; + const auto &iter = _exist_apps.find(app_name); + CHECK_TRUE(iter != _exist_apps.end()); + + iter->second->status = app_status::AS_DROPPING; return; } }); + zauto_read_lock l(_lock); + + const auto &app = get_app(app_name); if (!app) { LOG_WARNING("set app envs failed since app_name({}) cannot be found", app_name); env_rpc.response().err = ERR_APP_NOT_EXIST; @@ -3068,15 +3072,16 @@ void server_state::set_app_envs(const app_env_rpc &env_rpc) CHECK_EQ_MSG(ec, ERR_OK, "update app({}) info to remote storage failed", app_name); zauto_write_lock l(_lock); - std::shared_ptr app = get_app(app_name); - FAIL_POINT_INJECT_NOT_RETURN_F("set_app_envs_failed", [&app](std::string_view s) { - if (s == "dropped_after_update_remote_storage") { - app.reset(); + FAIL_POINT_INJECT_NOT_RETURN_F("set_app_envs_failed", [app_name, this](std::string_view s) { + if (s == "dropped_after") { + CHECK_EQ(_exist_apps.erase(app_name), 1); return; } }); + std::shared_ptr app = get_app(app_name); + // The table might be removed just before the callback function is invoked, thus we must // check if this table still exists. // diff --git a/src/meta/test/server_state_test.cpp b/src/meta/test/server_state_test.cpp index ce38ec6440..dde43b1ee1 100644 --- a/src/meta/test/server_state_test.cpp +++ b/src/meta/test/server_state_test.cpp @@ -207,7 +207,7 @@ class server_state_test dsn::message_ptr binary_req(dsn::message_ex::create_request(RPC_CM_UPDATE_APP_ENV)); dsn::marshall(binary_req, request); dsn::message_ex *recv_msg = create_corresponding_receive(binary_req); - return app_env_rpc(recv_msg); // don't need reply + return app_env_rpc(recv_msg); // Don't need to reply. } std::unique_ptr _ms; @@ -220,7 +220,7 @@ void meta_service_test_app::app_envs_basic_test() test.load_apps({"test_app1", "test_set_app_envs_not_found", "test_set_app_envs_dropping", - "test_set_app_envs_dropped_after_update_remote_storage"}); + "test_set_app_envs_dropped_after"}); #define TEST_SET_APP_ENVS_FAILED(action, err_code) \ std::cout << "test server_state::set_app_envs(" #action ")..." << std::endl; \ @@ -244,7 +244,7 @@ void meta_service_test_app::app_envs_basic_test() TEST_SET_APP_ENVS_FAILED(dropping, ERR_BUSY_DROPPING); - TEST_SET_APP_ENVS_FAILED(dropped_after_update_remote_storage, ERR_APP_DROPPED); + TEST_SET_APP_ENVS_FAILED(dropped_after, ERR_APP_DROPPED); #undef TEST_SET_APP_ENVS_FAILED @@ -264,6 +264,8 @@ void meta_service_test_app::app_envs_basic_test() for (size_t idx = 0; idx < keys.size(); ++idx) { const auto &key = keys[idx]; + + // Every env should be inserted. ASSERT_EQ(1, app->envs.count(key)); ASSERT_EQ(values[idx], app->envs.at(key)); } @@ -284,18 +286,21 @@ void meta_service_test_app::app_envs_basic_test() for (size_t idx = 0; idx < keys.size(); ++idx) { const std::string &key = keys[idx]; - if (del_keys_set.count(key) >= 1) { + if (del_keys_set.count(key) > 0) { + // The env in `del_keys_set` should be deleted. ASSERT_EQ(0, app->envs.count(key)); - } else { - ASSERT_EQ(1, app->envs.count(key)); - ASSERT_EQ(values[idx], app->envs.at(key)); + continue; } + + // The env should still exist if it is not in `del_keys_set`. + ASSERT_EQ(1, app->envs.count(key)); + ASSERT_EQ(values[idx], app->envs.at(key)); } } std::cout << "test server_state::clear_app_envs()..." << std::endl; { - // test specify prefix + // Test specifying prefix. { configuration_update_app_env_request request; request.__set_app_name("test_app1"); @@ -310,21 +315,25 @@ void meta_service_test_app::app_envs_basic_test() for (size_t idx = 0; idx < keys.size(); ++idx) { const std::string &key = keys[idx]; - if (del_keys_set.count(key) <= 0) { - if (acquire_prefix(key) == clear_prefix) { - ASSERT_EQ(0, app->envs.count(key)); - } else { - ASSERT_EQ(1, app->envs.count(key)); - ASSERT_EQ(values[idx], app->envs.at(key)); - } - } else { - // key already delete + if (del_keys_set.count(key) > 0) { + // The env should have been deleted during test for `del_app_envs`. ASSERT_EQ(0, app->envs.count(key)); + continue; } + + if (acquire_prefix(key) == clear_prefix) { + // The env with specified prefix should be deleted. + ASSERT_EQ(0, app->envs.count(key)); + continue; + } + + // Otherwise, the env should still exist. + ASSERT_EQ(1, app->envs.count(key)); + ASSERT_EQ(values[idx], app->envs.at(key)); } } - // test clear all + // Test clearing all. { configuration_update_app_env_request request; request.__set_app_name("test_app1"); @@ -337,6 +346,7 @@ void meta_service_test_app::app_envs_basic_test() const auto &app = test.get_app("test_app1"); ASSERT_TRUE(app); + // All envs should be cleared. ASSERT_TRUE(app->envs.empty()); } }