acelyc111 commented on code in PR #1960:
URL:
https://github.com/apache/incubator-pegasus/pull/1960#discussion_r1540940200
##########
src/meta/duplication/meta_duplication_service.cpp:
##########
@@ -158,76 +184,125 @@ void
meta_duplication_service::add_duplication(duplication_add_rpc rpc)
const auto &request = rpc.request();
auto &response = rpc.response();
- LOG_INFO("add duplication for app({}), remote cluster name is {}",
+ std::string remote_app_name;
+ if (request.__isset.remote_app_name) {
+ remote_app_name = request.remote_app_name;
+ } else {
+ // Once remote_app_name is not specified by client, use source
app_name as
+ // remote_app_name to be compatible with old versions(< v2.6.0) of
client.
+ remote_app_name = request.app_name;
+ }
+
+ LOG_INFO("add duplication for app({}), remote cluster name is {}"
+ "remote app name is {}",
Review Comment:
```suggestion
", remote app name is {}",
```
##########
src/meta/duplication/meta_duplication_service.cpp:
##########
@@ -158,76 +184,125 @@ void
meta_duplication_service::add_duplication(duplication_add_rpc rpc)
const auto &request = rpc.request();
auto &response = rpc.response();
- LOG_INFO("add duplication for app({}), remote cluster name is {}",
+ std::string remote_app_name;
+ if (request.__isset.remote_app_name) {
+ remote_app_name = request.remote_app_name;
+ } else {
+ // Once remote_app_name is not specified by client, use source
app_name as
+ // remote_app_name to be compatible with old versions(< v2.6.0) of
client.
+ remote_app_name = request.app_name;
+ }
+
+ LOG_INFO("add duplication for app({}), remote cluster name is {}"
+ "remote app name is {}",
request.app_name,
- request.remote_cluster_name);
+ request.remote_cluster_name,
+ remote_app_name);
- response.err = ERR_OK;
+ LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(request.remote_cluster_name !=
+ get_current_cluster_name(),
+ response,
+ "illegal operation: adding
duplication to itself");
- if (request.remote_cluster_name == get_current_cluster_name()) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint("illegal operation: adding duplication to itself");
- return;
- }
auto remote_cluster_id =
get_duplication_cluster_id(request.remote_cluster_name);
- if (!remote_cluster_id.is_ok()) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint(fmt::format("get_duplication_cluster_id({})
failed, error: {}",
- request.remote_cluster_name,
- remote_cluster_id.get_error()));
- return;
- }
+ LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(remote_cluster_id.is_ok(),
+ response,
+ "get_duplication_cluster_id({})
failed, error: {}",
+ request.remote_cluster_name,
+ remote_cluster_id.get_error());
std::vector<host_port> meta_list;
- if (!dsn::replication::replica_helper::load_meta_servers(
- meta_list,
- duplication_constants::kClustersSectionName.c_str(),
- request.remote_cluster_name.c_str())) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint(fmt::format("failed to find cluster[{}] address in
config [{}]",
- request.remote_cluster_name,
-
duplication_constants::kClustersSectionName));
- return;
- }
-
- auto app = _state->get_app(request.app_name);
- if (!app || app->status != app_status::AS_AVAILABLE) {
- response.err = ERR_APP_NOT_EXIST;
- return;
- }
+
LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(dsn::replication::replica_helper::load_meta_servers(
+ meta_list,
+
duplication_constants::kClustersSectionName.c_str(),
+
request.remote_cluster_name.c_str()),
+ response,
+ "failed to find cluster[{}] address
in config [{}]",
+ request.remote_cluster_name,
+
duplication_constants::kClustersSectionName);
+
+ std::shared_ptr<app_state> app;
duplication_info_s_ptr dup;
- for (const auto &ent : app->duplications) {
- auto it = ent.second;
- if (it->follower_cluster_name == request.remote_cluster_name) {
- dup = ent.second;
- break;
+ {
+ zauto_read_lock l(app_lock());
+
+ app = _state->get_app(request.app_name);
+ if (!app || app->status != app_status::AS_AVAILABLE) {
Review Comment:
Can reuse LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT?
##########
src/meta/duplication/meta_duplication_service.cpp:
##########
@@ -158,76 +184,125 @@ void
meta_duplication_service::add_duplication(duplication_add_rpc rpc)
const auto &request = rpc.request();
auto &response = rpc.response();
- LOG_INFO("add duplication for app({}), remote cluster name is {}",
+ std::string remote_app_name;
+ if (request.__isset.remote_app_name) {
+ remote_app_name = request.remote_app_name;
+ } else {
+ // Once remote_app_name is not specified by client, use source
app_name as
+ // remote_app_name to be compatible with old versions(< v2.6.0) of
client.
+ remote_app_name = request.app_name;
+ }
+
+ LOG_INFO("add duplication for app({}), remote cluster name is {}"
+ "remote app name is {}",
request.app_name,
- request.remote_cluster_name);
+ request.remote_cluster_name,
+ remote_app_name);
- response.err = ERR_OK;
+ LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(request.remote_cluster_name !=
+ get_current_cluster_name(),
+ response,
+ "illegal operation: adding
duplication to itself");
- if (request.remote_cluster_name == get_current_cluster_name()) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint("illegal operation: adding duplication to itself");
- return;
- }
auto remote_cluster_id =
get_duplication_cluster_id(request.remote_cluster_name);
- if (!remote_cluster_id.is_ok()) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint(fmt::format("get_duplication_cluster_id({})
failed, error: {}",
- request.remote_cluster_name,
- remote_cluster_id.get_error()));
- return;
- }
+ LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(remote_cluster_id.is_ok(),
+ response,
+ "get_duplication_cluster_id({})
failed, error: {}",
+ request.remote_cluster_name,
+ remote_cluster_id.get_error());
std::vector<host_port> meta_list;
- if (!dsn::replication::replica_helper::load_meta_servers(
- meta_list,
- duplication_constants::kClustersSectionName.c_str(),
- request.remote_cluster_name.c_str())) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint(fmt::format("failed to find cluster[{}] address in
config [{}]",
- request.remote_cluster_name,
-
duplication_constants::kClustersSectionName));
- return;
- }
-
- auto app = _state->get_app(request.app_name);
- if (!app || app->status != app_status::AS_AVAILABLE) {
- response.err = ERR_APP_NOT_EXIST;
- return;
- }
+
LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(dsn::replication::replica_helper::load_meta_servers(
+ meta_list,
+
duplication_constants::kClustersSectionName.c_str(),
+
request.remote_cluster_name.c_str()),
+ response,
+ "failed to find cluster[{}] address
in config [{}]",
+ request.remote_cluster_name,
+
duplication_constants::kClustersSectionName);
+
+ std::shared_ptr<app_state> app;
duplication_info_s_ptr dup;
- for (const auto &ent : app->duplications) {
- auto it = ent.second;
- if (it->follower_cluster_name == request.remote_cluster_name) {
- dup = ent.second;
- break;
+ {
+ zauto_read_lock l(app_lock());
+
+ app = _state->get_app(request.app_name);
+ if (!app || app->status != app_status::AS_AVAILABLE) {
+ response.err = ERR_APP_NOT_EXIST;
+ LOG_WARNING("app {} was not found", request.app_name);
+ return;
+ }
+
+ for (const auto &ent : app->duplications) {
Review Comment:
```suggestion
for (const auto & [ _, dup_info] : app->duplications) {
```
##########
src/meta/duplication/meta_duplication_service.cpp:
##########
@@ -158,76 +184,125 @@ void
meta_duplication_service::add_duplication(duplication_add_rpc rpc)
const auto &request = rpc.request();
auto &response = rpc.response();
- LOG_INFO("add duplication for app({}), remote cluster name is {}",
+ std::string remote_app_name;
+ if (request.__isset.remote_app_name) {
+ remote_app_name = request.remote_app_name;
+ } else {
+ // Once remote_app_name is not specified by client, use source
app_name as
+ // remote_app_name to be compatible with old versions(< v2.6.0) of
client.
+ remote_app_name = request.app_name;
+ }
+
+ LOG_INFO("add duplication for app({}), remote cluster name is {}"
+ "remote app name is {}",
request.app_name,
- request.remote_cluster_name);
+ request.remote_cluster_name,
+ remote_app_name);
- response.err = ERR_OK;
+ LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(request.remote_cluster_name !=
+ get_current_cluster_name(),
+ response,
+ "illegal operation: adding
duplication to itself");
- if (request.remote_cluster_name == get_current_cluster_name()) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint("illegal operation: adding duplication to itself");
- return;
- }
auto remote_cluster_id =
get_duplication_cluster_id(request.remote_cluster_name);
- if (!remote_cluster_id.is_ok()) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint(fmt::format("get_duplication_cluster_id({})
failed, error: {}",
- request.remote_cluster_name,
- remote_cluster_id.get_error()));
- return;
- }
+ LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(remote_cluster_id.is_ok(),
+ response,
+ "get_duplication_cluster_id({})
failed, error: {}",
+ request.remote_cluster_name,
+ remote_cluster_id.get_error());
std::vector<host_port> meta_list;
- if (!dsn::replication::replica_helper::load_meta_servers(
- meta_list,
- duplication_constants::kClustersSectionName.c_str(),
- request.remote_cluster_name.c_str())) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint(fmt::format("failed to find cluster[{}] address in
config [{}]",
- request.remote_cluster_name,
-
duplication_constants::kClustersSectionName));
- return;
- }
-
- auto app = _state->get_app(request.app_name);
- if (!app || app->status != app_status::AS_AVAILABLE) {
- response.err = ERR_APP_NOT_EXIST;
- return;
- }
+
LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(dsn::replication::replica_helper::load_meta_servers(
+ meta_list,
+
duplication_constants::kClustersSectionName.c_str(),
+
request.remote_cluster_name.c_str()),
+ response,
+ "failed to find cluster[{}] address
in config [{}]",
+ request.remote_cluster_name,
+
duplication_constants::kClustersSectionName);
+
+ std::shared_ptr<app_state> app;
duplication_info_s_ptr dup;
- for (const auto &ent : app->duplications) {
- auto it = ent.second;
- if (it->follower_cluster_name == request.remote_cluster_name) {
- dup = ent.second;
- break;
+ {
+ zauto_read_lock l(app_lock());
+
+ app = _state->get_app(request.app_name);
+ if (!app || app->status != app_status::AS_AVAILABLE) {
+ response.err = ERR_APP_NOT_EXIST;
+ LOG_WARNING("app {} was not found", request.app_name);
+ return;
+ }
+
+ for (const auto &ent : app->duplications) {
+ if (ent.second->remote_cluster_name ==
request.remote_cluster_name) {
+ dup = ent.second;
+ break;
+ }
+ }
+
+ if (dup) {
+ // The duplication for the same app to the same remote cluster has
existed.
+ remote_app_name = dup->remote_app_name;
+ LOG_INFO("no need to add duplication, since it has existed:
app_name={}, "
+ "remote_cluster_name={}, remote_app_name={}",
+ request.app_name,
+ request.remote_cluster_name,
+ remote_app_name);
+ } else {
+ // Check if other apps of this cluster are duplicated to the same
remote app.
+ for (const auto & [ app_name, cur_app_state ] :
_state->_exist_apps) {
+ if (app_name == request.app_name) {
+ // Skip this app since we want to check other apps.
+ continue;
+ }
+
+ for (const auto &ent : cur_app_state->duplications) {
+ LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(
+ ent.second->remote_cluster_name !=
request.remote_cluster_name ||
Review Comment:
should use `&&` rather than `||` ?
##########
src/meta/duplication/meta_duplication_service.cpp:
##########
@@ -158,76 +184,125 @@ void
meta_duplication_service::add_duplication(duplication_add_rpc rpc)
const auto &request = rpc.request();
auto &response = rpc.response();
- LOG_INFO("add duplication for app({}), remote cluster name is {}",
+ std::string remote_app_name;
+ if (request.__isset.remote_app_name) {
+ remote_app_name = request.remote_app_name;
+ } else {
+ // Once remote_app_name is not specified by client, use source
app_name as
+ // remote_app_name to be compatible with old versions(< v2.6.0) of
client.
+ remote_app_name = request.app_name;
+ }
+
+ LOG_INFO("add duplication for app({}), remote cluster name is {}"
+ "remote app name is {}",
request.app_name,
- request.remote_cluster_name);
+ request.remote_cluster_name,
+ remote_app_name);
- response.err = ERR_OK;
+ LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(request.remote_cluster_name !=
+ get_current_cluster_name(),
+ response,
+ "illegal operation: adding
duplication to itself");
- if (request.remote_cluster_name == get_current_cluster_name()) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint("illegal operation: adding duplication to itself");
- return;
- }
auto remote_cluster_id =
get_duplication_cluster_id(request.remote_cluster_name);
- if (!remote_cluster_id.is_ok()) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint(fmt::format("get_duplication_cluster_id({})
failed, error: {}",
- request.remote_cluster_name,
- remote_cluster_id.get_error()));
- return;
- }
+ LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(remote_cluster_id.is_ok(),
+ response,
+ "get_duplication_cluster_id({})
failed, error: {}",
+ request.remote_cluster_name,
+ remote_cluster_id.get_error());
std::vector<host_port> meta_list;
- if (!dsn::replication::replica_helper::load_meta_servers(
- meta_list,
- duplication_constants::kClustersSectionName.c_str(),
- request.remote_cluster_name.c_str())) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint(fmt::format("failed to find cluster[{}] address in
config [{}]",
- request.remote_cluster_name,
-
duplication_constants::kClustersSectionName));
- return;
- }
-
- auto app = _state->get_app(request.app_name);
- if (!app || app->status != app_status::AS_AVAILABLE) {
- response.err = ERR_APP_NOT_EXIST;
- return;
- }
+
LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(dsn::replication::replica_helper::load_meta_servers(
+ meta_list,
+
duplication_constants::kClustersSectionName.c_str(),
+
request.remote_cluster_name.c_str()),
+ response,
+ "failed to find cluster[{}] address
in config [{}]",
+ request.remote_cluster_name,
+
duplication_constants::kClustersSectionName);
+
+ std::shared_ptr<app_state> app;
duplication_info_s_ptr dup;
- for (const auto &ent : app->duplications) {
- auto it = ent.second;
- if (it->follower_cluster_name == request.remote_cluster_name) {
- dup = ent.second;
- break;
+ {
+ zauto_read_lock l(app_lock());
+
+ app = _state->get_app(request.app_name);
+ if (!app || app->status != app_status::AS_AVAILABLE) {
+ response.err = ERR_APP_NOT_EXIST;
+ LOG_WARNING("app {} was not found", request.app_name);
+ return;
+ }
+
+ for (const auto &ent : app->duplications) {
+ if (ent.second->remote_cluster_name ==
request.remote_cluster_name) {
+ dup = ent.second;
+ break;
+ }
+ }
+
+ if (dup) {
+ // The duplication for the same app to the same remote cluster has
existed.
+ remote_app_name = dup->remote_app_name;
+ LOG_INFO("no need to add duplication, since it has existed:
app_name={}, "
+ "remote_cluster_name={}, remote_app_name={}",
+ request.app_name,
+ request.remote_cluster_name,
+ remote_app_name);
+ } else {
Review Comment:
`continue` in `if` statement and you can remove the `else` to reduce the
indents.
##########
src/meta/duplication/meta_duplication_service.cpp:
##########
@@ -158,76 +184,125 @@ void
meta_duplication_service::add_duplication(duplication_add_rpc rpc)
const auto &request = rpc.request();
auto &response = rpc.response();
- LOG_INFO("add duplication for app({}), remote cluster name is {}",
+ std::string remote_app_name;
+ if (request.__isset.remote_app_name) {
+ remote_app_name = request.remote_app_name;
+ } else {
+ // Once remote_app_name is not specified by client, use source
app_name as
+ // remote_app_name to be compatible with old versions(< v2.6.0) of
client.
+ remote_app_name = request.app_name;
+ }
+
+ LOG_INFO("add duplication for app({}), remote cluster name is {}"
+ "remote app name is {}",
request.app_name,
- request.remote_cluster_name);
+ request.remote_cluster_name,
+ remote_app_name);
- response.err = ERR_OK;
+ LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(request.remote_cluster_name !=
+ get_current_cluster_name(),
+ response,
+ "illegal operation: adding
duplication to itself");
- if (request.remote_cluster_name == get_current_cluster_name()) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint("illegal operation: adding duplication to itself");
- return;
- }
auto remote_cluster_id =
get_duplication_cluster_id(request.remote_cluster_name);
- if (!remote_cluster_id.is_ok()) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint(fmt::format("get_duplication_cluster_id({})
failed, error: {}",
- request.remote_cluster_name,
- remote_cluster_id.get_error()));
- return;
- }
+ LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(remote_cluster_id.is_ok(),
+ response,
+ "get_duplication_cluster_id({})
failed, error: {}",
+ request.remote_cluster_name,
+ remote_cluster_id.get_error());
std::vector<host_port> meta_list;
- if (!dsn::replication::replica_helper::load_meta_servers(
- meta_list,
- duplication_constants::kClustersSectionName.c_str(),
- request.remote_cluster_name.c_str())) {
- response.err = ERR_INVALID_PARAMETERS;
- response.__set_hint(fmt::format("failed to find cluster[{}] address in
config [{}]",
- request.remote_cluster_name,
-
duplication_constants::kClustersSectionName));
- return;
- }
-
- auto app = _state->get_app(request.app_name);
- if (!app || app->status != app_status::AS_AVAILABLE) {
- response.err = ERR_APP_NOT_EXIST;
- return;
- }
+
LOG_WARNING_DUP_HINT_AND_RETURN_IF_NOT(dsn::replication::replica_helper::load_meta_servers(
+ meta_list,
+
duplication_constants::kClustersSectionName.c_str(),
+
request.remote_cluster_name.c_str()),
+ response,
+ "failed to find cluster[{}] address
in config [{}]",
+ request.remote_cluster_name,
+
duplication_constants::kClustersSectionName);
+
+ std::shared_ptr<app_state> app;
duplication_info_s_ptr dup;
- for (const auto &ent : app->duplications) {
- auto it = ent.second;
- if (it->follower_cluster_name == request.remote_cluster_name) {
- dup = ent.second;
- break;
+ {
+ zauto_read_lock l(app_lock());
+
+ app = _state->get_app(request.app_name);
+ if (!app || app->status != app_status::AS_AVAILABLE) {
+ response.err = ERR_APP_NOT_EXIST;
+ LOG_WARNING("app {} was not found", request.app_name);
Review Comment:
Because it also judge the `app->status`, so it's better to indicate this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
