You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2022/11/03 13:02:42 UTC
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1217: refactor(macro): use CHECK_* to replace dassert_* (part 2)
empiredan commented on code in PR #1217:
URL: https://github.com/apache/incubator-pegasus/pull/1217#discussion_r1012461814
##########
src/common/storage_serverlet.h:
##########
@@ -105,10 +105,10 @@ class storage_serverlet
dassert(s_handlers.emplace(name, h).second, "handler %s has already been registered", name);
s_vhandlers.resize(rpc_code + 1);
- dassert(s_vhandlers[rpc_code] == nullptr,
- "handler %s(%d) has already been registered",
- rpc_code.to_string(),
- rpc_code.code());
+ CHECK(s_vhandlers[rpc_code] == nullptr,
+ "handler {}({}) has already been registered",
+ rpc_code.to_string(),
Review Comment:
```suggestion
rpc_code,
```
##########
src/common/duplication_common.cpp:
##########
@@ -96,20 +96,22 @@ class duplication_group_registry : public utils::singleton<duplication_group_reg
for (std::string &cluster : clusters) {
int64_t cluster_id =
dsn_config_get_value_int64("duplication-group", cluster.data(), 0, "");
- dassert(cluster_id < 128 && cluster_id > 0,
- "cluster_id(%zd) for %s should be in [1, 127]",
- cluster_id,
- cluster.data());
+ CHECK(cluster_id < 128 && cluster_id > 0,
+ "cluster_id({}) for {} should be in [1, 127]",
+ cluster_id,
+ cluster.data());
Review Comment:
```suggestion
cluster);
```
##########
src/meta/meta_backup_service.cpp:
##########
@@ -490,28 +490,23 @@ void policy_context::on_backup_reply(error_code err,
pid.to_string(),
primary.to_string());
if (err == dsn::ERR_OK && response.err == dsn::ERR_OK) {
- dassert(response.policy_name == _policy.policy_name,
- "policy name(%s vs %s) don't match, pid(%d.%d), replica_server(%s)",
- _policy.policy_name.c_str(),
- response.policy_name.c_str(),
- pid.get_app_id(),
- pid.get_partition_index(),
- primary.to_string());
- dassert(response.pid == pid,
- "%s: backup pid[(%d.%d) vs (%d.%d)] don't match, replica_server(%s)",
- _policy.policy_name.c_str(),
- response.pid.get_app_id(),
- response.pid.get_partition_index(),
- pid.get_app_id(),
- pid.get_partition_index(),
- primary.to_string());
- dassert(response.backup_id <= _cur_backup.backup_id,
- "%s: replica server(%s) has bigger backup_id(%lld), gpid(%d.%d)",
- _backup_sig.c_str(),
- primary.to_string(),
- response.backup_id,
- pid.get_app_id(),
- pid.get_partition_index());
+ CHECK_EQ_MSG(response.policy_name,
+ _policy.policy_name,
+ "policy names don't match, pid({}), replica_server({})",
+ pid,
+ primary.to_string());
Review Comment:
```suggestion
primary);
```
##########
src/aio/disk_engine.cpp:
##########
@@ -107,7 +107,7 @@ aio_task *disk_file::write(aio_task *tsk, void *ctx)
aio_task *disk_file::on_read_completed(aio_task *wk, error_code err, size_t size)
{
- dassert(wk->next == nullptr, "");
+ CHECK(wk->next == nullptr, "");
Review Comment:
```suggestion
CHECK_EQ(wk->next, nullptr);
```
##########
src/meta/load_balance_policy.cpp:
##########
@@ -278,12 +279,13 @@ void load_balance_policy::start_moving_primary(const std::shared_ptr<app_state>
{
std::list<dsn::gpid> potential_moving = calc_potential_moving(app, from, to);
auto potential_moving_size = potential_moving.size();
- dassert_f(plan_moving <= potential_moving_size,
- "from({}) to({}) plan({}), can_move({})",
- from.to_string(),
- to.to_string(),
- plan_moving,
- potential_moving_size);
+ CHECK_LE_MSG(plan_moving,
+ potential_moving_size,
+ "from({}) to({}) plan({}), can_move({})",
+ from.to_string(),
Review Comment:
```suggestion
from,
```
##########
src/meta/meta_backup_service.cpp:
##########
@@ -490,28 +490,23 @@ void policy_context::on_backup_reply(error_code err,
pid.to_string(),
primary.to_string());
if (err == dsn::ERR_OK && response.err == dsn::ERR_OK) {
- dassert(response.policy_name == _policy.policy_name,
- "policy name(%s vs %s) don't match, pid(%d.%d), replica_server(%s)",
- _policy.policy_name.c_str(),
- response.policy_name.c_str(),
- pid.get_app_id(),
- pid.get_partition_index(),
- primary.to_string());
- dassert(response.pid == pid,
- "%s: backup pid[(%d.%d) vs (%d.%d)] don't match, replica_server(%s)",
- _policy.policy_name.c_str(),
- response.pid.get_app_id(),
- response.pid.get_partition_index(),
- pid.get_app_id(),
- pid.get_partition_index(),
- primary.to_string());
- dassert(response.backup_id <= _cur_backup.backup_id,
- "%s: replica server(%s) has bigger backup_id(%lld), gpid(%d.%d)",
- _backup_sig.c_str(),
- primary.to_string(),
- response.backup_id,
- pid.get_app_id(),
- pid.get_partition_index());
+ CHECK_EQ_MSG(response.policy_name,
+ _policy.policy_name,
+ "policy names don't match, pid({}), replica_server({})",
+ pid,
+ primary.to_string());
+ CHECK_EQ_MSG(response.pid,
+ pid,
+ "{}: backup pids don't match, replica_server({})",
+ _policy.policy_name,
+ primary.to_string());
+ CHECK_LE_MSG(response.backup_id,
+ _cur_backup.backup_id,
+ "{}: replica server({}) has bigger backup_id({}), gpid({})",
+ _backup_sig,
+ primary.to_string(),
Review Comment:
```suggestion
primary,
```
##########
src/meta/meta_data.cpp:
##########
@@ -118,9 +118,9 @@ void maintain_drops(std::vector<rpc_address> &drops, const rpc_address &node, co
drops.erase(it);
}
} else {
- dassert(it == drops.end(),
- "the node(%s) cannot be in drops set before this update",
- node.to_string());
+ CHECK(it == drops.end(),
+ "the node({}) cannot be in drops set before this update",
+ node.to_string());
Review Comment:
```suggestion
node);
```
##########
src/meta/server_state.cpp:
##########
@@ -1049,11 +1048,11 @@ void server_state::init_app_partition_node(std::shared_ptr<app_state> &app,
0,
std::chrono::milliseconds(1000));
} else {
- dassert(false,
- "we can't handle this error in init app partition nodes err(%s), gpid(%d.%d)",
- ec.to_string(),
- app->app_id,
- pidx);
+ CHECK(false,
+ "we can't handle this error in init app partition nodes err({}), gpid({}.{})",
+ ec.to_string(),
Review Comment:
```suggestion
ec,
```
##########
src/meta/server_load_balancer.cpp:
##########
@@ -114,13 +114,13 @@ void newly_partitions::newly_remove_primary(int32_t app_id, bool only_primary)
void newly_partitions::newly_remove_partition(int32_t app_id)
{
auto iter = partitions.find(app_id);
- dassert(iter != partitions.end(), "invalid app_id, app_id = %d", app_id);
- dassert(iter->second > 0, "invalid partition count, cnt = %d", iter->second);
+ CHECK(iter != partitions.end(), "invalid app_id, app_id = {}", app_id);
+ CHECK_GT_MSG(iter->second, 0, "invalid partition coun");
Review Comment:
```suggestion
CHECK_GT_MSG(iter->second, 0, "invalid partition count");
```
##########
src/meta/server_state.cpp:
##########
@@ -1204,7 +1203,7 @@ void server_state::do_app_drop(std::shared_ptr<app_state> &app)
0,
std::chrono::seconds(1));
} else {
- dassert(false, "we can't handle this, error(%s)", ec.to_string());
+ CHECK(false, "we can't handle this, error({})", ec.to_string());
Review Comment:
```suggestion
CHECK(false, "we can't handle this, error({})", ec);
```
##########
src/meta/server_state.cpp:
##########
@@ -1543,9 +1517,9 @@ void server_state::update_configuration_locally(
}
auto it = _nodes.find(config_request->host_node);
- dassert(it != _nodes.end(),
- "invalid node address, address = %s",
- config_request->host_node.to_string());
+ CHECK(it != _nodes.end(),
+ "invalid node address, address = {}",
+ config_request->host_node.to_string());
Review Comment:
```suggestion
config_request->host_node);
```
##########
src/meta/server_state.cpp:
##########
@@ -1460,18 +1439,16 @@ void server_state::update_configuration_locally(
health_status new_health_status = partition_health_status(new_cfg, min_2pc_count);
if (app.is_stateful) {
- dassert(old_cfg.ballot == invalid_ballot || old_cfg.ballot + 1 == new_cfg.ballot,
- "invalid configuration update request, old ballot %" PRId64 ", new ballot %" PRId64
- "",
- old_cfg.ballot,
- new_cfg.ballot);
+ CHECK(old_cfg.ballot == invalid_ballot || old_cfg.ballot + 1 == new_cfg.ballot,
+ "invalid configuration update request, old ballot {}, new ballot {}",
+ old_cfg.ballot,
+ new_cfg.ballot);
node_state *ns = nullptr;
if (config_request->type != config_type::CT_DROP_PARTITION) {
ns = get_node_state(_nodes, config_request->node, false);
- dassert(ns != nullptr,
- "invalid node address, address = %s",
- config_request->node.to_string());
+ CHECK_NOTNULL(
+ ns, "invalid node address, address = {}", config_request->node.to_string());
Review Comment:
```suggestion
ns, "invalid node address, address = {}", config_request->node);
```
##########
src/meta/load_balance_policy.cpp:
##########
@@ -278,12 +279,13 @@ void load_balance_policy::start_moving_primary(const std::shared_ptr<app_state>
{
std::list<dsn::gpid> potential_moving = calc_potential_moving(app, from, to);
auto potential_moving_size = potential_moving.size();
- dassert_f(plan_moving <= potential_moving_size,
- "from({}) to({}) plan({}), can_move({})",
- from.to_string(),
- to.to_string(),
- plan_moving,
- potential_moving_size);
+ CHECK_LE_MSG(plan_moving,
+ potential_moving_size,
+ "from({}) to({}) plan({}), can_move({})",
+ from.to_string(),
+ to.to_string(),
Review Comment:
```suggestion
to,
```
##########
src/meta/server_state.cpp:
##########
@@ -1677,7 +1651,7 @@ void server_state::on_update_configuration_on_remote_reply(
}
}
} else {
- dassert(false, "we can't handle this right now, err = %s", ec.to_string());
+ CHECK(false, "we can't handle this right now, err = {}", ec.to_string());
Review Comment:
```suggestion
CHECK(false, "we can't handle this right now, err = {}", ec);
```
##########
src/meta/server_state.cpp:
##########
@@ -2606,55 +2577,40 @@ void server_state::get_cluster_balance_score(double &primary_stddev, double &tot
void server_state::check_consistency(const dsn::gpid &gpid)
{
auto iter = _all_apps.find(gpid.get_app_id());
- dassert(iter != _all_apps.end(),
- "invalid gpid(%d.%d)",
- gpid.get_app_id(),
- gpid.get_partition_index());
+ CHECK(iter != _all_apps.end(), "invalid gpid({})", gpid);
app_state &app = *(iter->second);
partition_configuration &config = app.partitions[gpid.get_partition_index()];
if (app.is_stateful) {
if (config.primary.is_invalid() == false) {
auto it = _nodes.find(config.primary);
- dassert(it != _nodes.end(),
- "invalid primary address, address = %s",
- config.primary.to_string());
- dassert(it->second.served_as(gpid) == partition_status::PS_PRIMARY,
- "node should serve as PS_PRIMARY, but status = %s",
- dsn::enum_to_string(it->second.served_as(gpid)));
-
- auto it2 =
- std::find(config.last_drops.begin(), config.last_drops.end(), config.primary);
- dassert(it2 == config.last_drops.end(),
- "primary shouldn't appear in last_drops, address = %s",
- config.primary.to_string());
+ CHECK(it != _nodes.end(),
+ "invalid primary address, address = {}",
+ config.primary.to_string());
+ CHECK_EQ(it->second.served_as(gpid), partition_status::PS_PRIMARY);
+ CHECK(std::find(config.last_drops.begin(), config.last_drops.end(), config.primary) ==
+ config.last_drops.end(),
+ "primary shouldn't appear in last_drops, address = {}",
+ config.primary.to_string());
}
for (auto &ep : config.secondaries) {
auto it = _nodes.find(ep);
- dassert(it != _nodes.end(), "invalid secondary address, address = %s", ep.to_string());
- dassert(it->second.served_as(gpid) == partition_status::PS_SECONDARY,
- "node should serve as PS_SECONDARY, but status = %s",
- dsn::enum_to_string(it->second.served_as(gpid)));
-
- auto it2 = std::find(config.last_drops.begin(), config.last_drops.end(), ep);
- dassert(it2 == config.last_drops.end(),
- "secondary shouldn't appear in last_drops, address = %s",
- ep.to_string());
+ CHECK(it != _nodes.end(), "invalid secondary address, address = {}", ep.to_string());
+ CHECK_EQ(it->second.served_as(gpid), partition_status::PS_SECONDARY);
+ CHECK(std::find(config.last_drops.begin(), config.last_drops.end(), ep) ==
+ config.last_drops.end(),
+ "secondary shouldn't appear in last_drops, address = {}",
+ ep.to_string());
}
} else {
partition_configuration_stateless pcs(config);
- dassert(pcs.hosts().size() == pcs.workers().size(),
- "%d VS %d",
- pcs.hosts().size(),
- pcs.workers().size());
+ CHECK_EQ(pcs.hosts().size(), pcs.workers().size());
for (auto &ep : pcs.hosts()) {
auto it = _nodes.find(ep);
- dassert(it != _nodes.end(), "invalid host, address = %s", ep.to_string());
- dassert(it->second.served_as(gpid) == partition_status::PS_SECONDARY,
- "node should serve as PS_SECONDARY, but status = %s",
- dsn::enum_to_string(it->second.served_as(gpid)));
+ CHECK(it != _nodes.end(), "invalid host, address = {}", ep.to_string());
Review Comment:
```suggestion
CHECK(it != _nodes.end(), "invalid host, address = {}", ep);
```
##########
src/meta/server_state.cpp:
##########
@@ -2606,55 +2577,40 @@ void server_state::get_cluster_balance_score(double &primary_stddev, double &tot
void server_state::check_consistency(const dsn::gpid &gpid)
{
auto iter = _all_apps.find(gpid.get_app_id());
- dassert(iter != _all_apps.end(),
- "invalid gpid(%d.%d)",
- gpid.get_app_id(),
- gpid.get_partition_index());
+ CHECK(iter != _all_apps.end(), "invalid gpid({})", gpid);
app_state &app = *(iter->second);
partition_configuration &config = app.partitions[gpid.get_partition_index()];
if (app.is_stateful) {
if (config.primary.is_invalid() == false) {
auto it = _nodes.find(config.primary);
- dassert(it != _nodes.end(),
- "invalid primary address, address = %s",
- config.primary.to_string());
- dassert(it->second.served_as(gpid) == partition_status::PS_PRIMARY,
- "node should serve as PS_PRIMARY, but status = %s",
- dsn::enum_to_string(it->second.served_as(gpid)));
-
- auto it2 =
- std::find(config.last_drops.begin(), config.last_drops.end(), config.primary);
- dassert(it2 == config.last_drops.end(),
- "primary shouldn't appear in last_drops, address = %s",
- config.primary.to_string());
+ CHECK(it != _nodes.end(),
+ "invalid primary address, address = {}",
+ config.primary.to_string());
+ CHECK_EQ(it->second.served_as(gpid), partition_status::PS_PRIMARY);
+ CHECK(std::find(config.last_drops.begin(), config.last_drops.end(), config.primary) ==
+ config.last_drops.end(),
+ "primary shouldn't appear in last_drops, address = {}",
+ config.primary.to_string());
}
for (auto &ep : config.secondaries) {
auto it = _nodes.find(ep);
- dassert(it != _nodes.end(), "invalid secondary address, address = %s", ep.to_string());
- dassert(it->second.served_as(gpid) == partition_status::PS_SECONDARY,
- "node should serve as PS_SECONDARY, but status = %s",
- dsn::enum_to_string(it->second.served_as(gpid)));
-
- auto it2 = std::find(config.last_drops.begin(), config.last_drops.end(), ep);
- dassert(it2 == config.last_drops.end(),
- "secondary shouldn't appear in last_drops, address = %s",
- ep.to_string());
+ CHECK(it != _nodes.end(), "invalid secondary address, address = {}", ep.to_string());
Review Comment:
```suggestion
CHECK(it != _nodes.end(), "invalid secondary address, address = {}", ep);
```
##########
src/meta/server_state.cpp:
##########
@@ -2606,55 +2577,40 @@ void server_state::get_cluster_balance_score(double &primary_stddev, double &tot
void server_state::check_consistency(const dsn::gpid &gpid)
{
auto iter = _all_apps.find(gpid.get_app_id());
- dassert(iter != _all_apps.end(),
- "invalid gpid(%d.%d)",
- gpid.get_app_id(),
- gpid.get_partition_index());
+ CHECK(iter != _all_apps.end(), "invalid gpid({})", gpid);
app_state &app = *(iter->second);
partition_configuration &config = app.partitions[gpid.get_partition_index()];
if (app.is_stateful) {
if (config.primary.is_invalid() == false) {
auto it = _nodes.find(config.primary);
- dassert(it != _nodes.end(),
- "invalid primary address, address = %s",
- config.primary.to_string());
- dassert(it->second.served_as(gpid) == partition_status::PS_PRIMARY,
- "node should serve as PS_PRIMARY, but status = %s",
- dsn::enum_to_string(it->second.served_as(gpid)));
-
- auto it2 =
- std::find(config.last_drops.begin(), config.last_drops.end(), config.primary);
- dassert(it2 == config.last_drops.end(),
- "primary shouldn't appear in last_drops, address = %s",
- config.primary.to_string());
+ CHECK(it != _nodes.end(),
+ "invalid primary address, address = {}",
+ config.primary.to_string());
+ CHECK_EQ(it->second.served_as(gpid), partition_status::PS_PRIMARY);
+ CHECK(std::find(config.last_drops.begin(), config.last_drops.end(), config.primary) ==
+ config.last_drops.end(),
+ "primary shouldn't appear in last_drops, address = {}",
+ config.primary.to_string());
}
for (auto &ep : config.secondaries) {
auto it = _nodes.find(ep);
- dassert(it != _nodes.end(), "invalid secondary address, address = %s", ep.to_string());
- dassert(it->second.served_as(gpid) == partition_status::PS_SECONDARY,
- "node should serve as PS_SECONDARY, but status = %s",
- dsn::enum_to_string(it->second.served_as(gpid)));
-
- auto it2 = std::find(config.last_drops.begin(), config.last_drops.end(), ep);
- dassert(it2 == config.last_drops.end(),
- "secondary shouldn't appear in last_drops, address = %s",
- ep.to_string());
+ CHECK(it != _nodes.end(), "invalid secondary address, address = {}", ep.to_string());
+ CHECK_EQ(it->second.served_as(gpid), partition_status::PS_SECONDARY);
+ CHECK(std::find(config.last_drops.begin(), config.last_drops.end(), ep) ==
+ config.last_drops.end(),
+ "secondary shouldn't appear in last_drops, address = {}",
+ ep.to_string());
Review Comment:
```suggestion
ep);
```
##########
src/replica/replica_config.cpp:
##########
@@ -158,30 +155,16 @@ void replica::add_potential_secondary(configuration_update_request &proposal)
return;
}
- dassert(proposal.config.ballot == get_ballot(),
- "invalid ballot, %" PRId64 " VS %" PRId64 "",
- proposal.config.ballot,
- get_ballot());
- dassert(proposal.config.pid == _primary_states.membership.pid,
- "(%d.%d) VS (%d.%d)",
- proposal.config.pid.get_app_id(),
- proposal.config.pid.get_partition_index(),
- _primary_states.membership.pid.get_app_id(),
- _primary_states.membership.pid.get_partition_index());
- dassert(proposal.config.primary == _primary_states.membership.primary,
- "%s VS %s",
- proposal.config.primary.to_string(),
- _primary_states.membership.primary.to_string());
- dassert(proposal.config.secondaries == _primary_states.membership.secondaries,
- "count(%d) VS count(%d)",
- (int)proposal.config.secondaries.size(),
- (int)_primary_states.membership.secondaries.size());
- dassert(!_primary_states.check_exist(proposal.node, partition_status::PS_PRIMARY),
- "node = %s",
- proposal.node.to_string());
- dassert(!_primary_states.check_exist(proposal.node, partition_status::PS_SECONDARY),
- "node = %s",
- proposal.node.to_string());
+ CHECK_EQ(proposal.config.ballot, get_ballot());
+ CHECK_EQ(proposal.config.pid, _primary_states.membership.pid);
+ CHECK_EQ(proposal.config.primary, _primary_states.membership.primary);
+ CHECK(proposal.config.secondaries == _primary_states.membership.secondaries, "");
+ CHECK(!_primary_states.check_exist(proposal.node, partition_status::PS_PRIMARY),
+ "node = {}",
+ proposal.node.to_string());
Review Comment:
```suggestion
proposal.node);
```
##########
src/utils/fmt_logging.h:
##########
@@ -59,12 +59,28 @@
// Macros to check expected condition. It will abort the application
// and log a fatal message when the condition is not met.
+#define CHECK_NE(var1, var2) CHECK(var1 != var2, "{} vs {}", var1, var2)
#define CHECK_EQ(var1, var2) CHECK(var1 == var2, "{} vs {}", var1, var2)
#define CHECK_GE(var1, var2) CHECK(var1 >= var2, "{} vs {}", var1, var2)
#define CHECK_LE(var1, var2) CHECK(var1 <= var2, "{} vs {}", var1, var2)
#define CHECK_GT(var1, var2) CHECK(var1 > var2, "{} vs {}", var1, var2)
#define CHECK_LT(var1, var2) CHECK(var1 < var2, "{} vs {}", var1, var2)
+#define CHECK_NE_MSG(var1, var2, ...) \
+ CHECK(var1 != var2, "{} vs {} {}", var1, var2, fmt::format(__VA_ARGS__))
+#define CHECK_EQ_MSG(var1, var2, ...) \
Review Comment:
Since `var1` of `var2` of " CHECK* might be a function or complex expression, both of them will be executed twice, which may lead to problems. Therefore it's better to execute each parameter and assign to another variable, for example, `auto a = (var1);`.
##########
src/replica/replica_config.cpp:
##########
@@ -311,31 +276,21 @@ void replica::remove(configuration_update_request &proposal)
if (proposal.config.ballot != get_ballot() || status() != partition_status::PS_PRIMARY)
return;
- dassert(proposal.config.pid == _primary_states.membership.pid,
- "(%d.%d) VS (%d.%d)",
- proposal.config.pid.get_app_id(),
- proposal.config.pid.get_partition_index(),
- _primary_states.membership.pid.get_app_id(),
- _primary_states.membership.pid.get_partition_index());
- dassert(proposal.config.primary == _primary_states.membership.primary,
- "%s VS %s",
- proposal.config.primary.to_string(),
- _primary_states.membership.primary.to_string());
- dassert(proposal.config.secondaries == _primary_states.membership.secondaries, "");
+ CHECK_EQ(proposal.config.pid, _primary_states.membership.pid);
+ CHECK_EQ(proposal.config.primary, _primary_states.membership.primary);
+ CHECK(proposal.config.secondaries == _primary_states.membership.secondaries, "");
auto st = _primary_states.get_node_status(proposal.node);
switch (st) {
case partition_status::PS_PRIMARY:
- dassert(proposal.config.primary == proposal.node,
- "%s VS %s",
- proposal.config.primary.to_string(),
- proposal.node.to_string());
+ CHECK_EQ(proposal.config.primary, proposal.node);
proposal.config.primary.set_invalid();
break;
case partition_status::PS_SECONDARY: {
- auto rt = replica_helper::remove_node(proposal.node, proposal.config.secondaries);
- dassert(rt, "remove_node failed, node = %s", proposal.node.to_string());
+ CHECK(replica_helper::remove_node(proposal.node, proposal.config.secondaries),
+ "remove_node failed, node = {}",
+ proposal.node.to_string());
Review Comment:
```suggestion
proposal.node);
```
##########
src/runtime/rpc/message_parser.cpp:
##########
@@ -161,12 +161,7 @@ char *message_reader::read_buffer_ptr(unsigned int read_next)
_buffer_occupied = rb.length();
}
- dassert(read_next + _buffer_occupied <= _buffer.length(),
- "%u(%u + %u) VS %u",
- read_next + _buffer_occupied,
- read_next,
- _buffer_occupied,
- _buffer.length());
+ CHECK_LE(read_next + _buffer_occupied, _buffer.length());
Review Comment:
Better to print each addend by `CHECK_LE_MSG` ?
##########
src/replica/replica_stub.cpp:
##########
@@ -649,21 +648,21 @@ void replica_stub::initialize(const replication_options &opts, bool clear /* = f
// restart log service
_log->close();
_log = nullptr;
- if (!utils::filesystem::remove_path(_options.slog_dir)) {
- dassert(false, "remove directory %s failed", _options.slog_dir.c_str());
- }
+ CHECK(utils::filesystem::remove_path(_options.slog_dir),
+ "remove directory {} failed",
+ _options.slog_dir);
_log = new mutation_log_shared(_options.slog_dir,
_options.log_shared_file_size_mb,
_options.log_shared_force_flush,
&_counter_shared_log_recent_write_size);
- auto lerr = _log->open(nullptr, [this](error_code err) { this->handle_log_failure(err); });
- dassert(lerr == ERR_OK, "restart log service must succeed");
+ CHECK_EQ_MSG(_log->open(nullptr, [this](error_code err) { this->handle_log_failure(err); }),
+ ERR_OK,
+ "restart log service failed");
}
bool is_log_complete = true;
for (auto it = rps.begin(); it != rps.end(); ++it) {
- auto err = it->second->background_sync_checkpoint();
- dassert(err == ERR_OK, "sync checkpoint failed, err = %s", err.to_string());
+ CHECK_EQ_MSG(it->second->background_sync_checkpoint(), ERR_OK, "sync checkpoint failed");
Review Comment:
May be executed twice, see [src/utils/fmt_logging.h](https://github.com/apache/incubator-pegasus/pull/1217/files#diff-02a287c86805a1841e2cfaffdbab494ec1b4c01e7fb8be1e6bd965c59ee020e2)
##########
src/meta/meta_data.cpp:
##########
@@ -135,12 +135,8 @@ bool construct_replica(meta_view view, const gpid &pid, int max_replica_count)
partition_configuration &pc = *get_config(*view.apps, pid);
config_context &cc = *get_config_context(*view.apps, pid);
- dassert(replica_count(pc) == 0,
- "replica count of gpid(%d.%d) must be 0",
- pid.get_app_id(),
- pid.get_partition_index());
- dassert(
- max_replica_count > 0, "max replica count is %d, should be at lease 1", max_replica_count);
+ CHECK_EQ(replica_count(pc), 0);
Review Comment:
```suggestion
CHECK_EQ_MSG(replica_count(pc), 0, "replica count of gpid({}) must be 0", pid);
```
##########
src/meta/server_state.cpp:
##########
@@ -745,7 +744,7 @@ void server_state::initialize_node_state()
ns->put_partition(pc.pid, true);
}
for (auto &ep : pc.secondaries) {
- dassert(!ep.is_invalid(), "invalid secondary address, addr = %s", ep.to_string());
+ CHECK(!ep.is_invalid(), "invalid secondary address, addr = {}", ep.to_string());
Review Comment:
```suggestion
CHECK(!ep.is_invalid(), "invalid secondary address, addr = {}", ep);
```
##########
src/meta/meta_backup_service.cpp:
##########
@@ -490,28 +490,23 @@ void policy_context::on_backup_reply(error_code err,
pid.to_string(),
primary.to_string());
if (err == dsn::ERR_OK && response.err == dsn::ERR_OK) {
- dassert(response.policy_name == _policy.policy_name,
- "policy name(%s vs %s) don't match, pid(%d.%d), replica_server(%s)",
- _policy.policy_name.c_str(),
- response.policy_name.c_str(),
- pid.get_app_id(),
- pid.get_partition_index(),
- primary.to_string());
- dassert(response.pid == pid,
- "%s: backup pid[(%d.%d) vs (%d.%d)] don't match, replica_server(%s)",
- _policy.policy_name.c_str(),
- response.pid.get_app_id(),
- response.pid.get_partition_index(),
- pid.get_app_id(),
- pid.get_partition_index(),
- primary.to_string());
- dassert(response.backup_id <= _cur_backup.backup_id,
- "%s: replica server(%s) has bigger backup_id(%lld), gpid(%d.%d)",
- _backup_sig.c_str(),
- primary.to_string(),
- response.backup_id,
- pid.get_app_id(),
- pid.get_partition_index());
+ CHECK_EQ_MSG(response.policy_name,
+ _policy.policy_name,
+ "policy names don't match, pid({}), replica_server({})",
+ pid,
+ primary.to_string());
+ CHECK_EQ_MSG(response.pid,
+ pid,
+ "{}: backup pids don't match, replica_server({})",
+ _policy.policy_name,
+ primary.to_string());
Review Comment:
```suggestion
primary);
```
##########
src/meta/server_state.cpp:
##########
@@ -1080,7 +1079,7 @@ void server_state::do_app_create(std::shared_ptr<app_state> &app)
0,
std::chrono::seconds(1));
} else {
- dassert(false, "we can't handle this right now, err(%s)", ec.to_string());
+ CHECK(false, "we can't handle this right now, err({})", ec.to_string());
Review Comment:
```suggestion
CHECK(false, "we can't handle this right now, err({})", ec);
```
##########
src/meta/server_state.cpp:
##########
@@ -890,11 +889,11 @@ void server_state::on_config_sync(configuration_query_by_node_rpc rpc)
rep.pid);
} else {
// app is not recognized or partition is not recognized
- dassert(false,
- "gpid({}) on node({}) is not exist on meta server, administrator "
- "should check consistency of meta data",
- rep.pid,
- request.node.to_string());
+ CHECK(false,
+ "gpid({}) on node({}) is not exist on meta server, administrator "
+ "should check consistency of meta data",
+ rep.pid,
+ request.node.to_string());
Review Comment:
```suggestion
request.node);
```
##########
src/meta/server_state.cpp:
##########
@@ -1961,9 +1936,9 @@ void server_state::on_partition_node_dead(std::shared_ptr<app_state> &app,
pc.pid.get_partition_index(),
address.to_string());
} else {
- dassert(false,
- "no primary/secondary on this node, node address = %s",
- address.to_string());
+ CHECK(false,
+ "no primary/secondary on this node, node address = {}",
+ address.to_string());
Review Comment:
```suggestion
address);
```
##########
src/meta/server_state.cpp:
##########
@@ -2050,15 +2025,12 @@ server_state::construct_apps(const std::vector<query_app_info_response> &query_a
{
// compatible for app.duplicating different between primary and secondaries in
// 2.1.x, 2.2.x and 2.3.x release
- if (!app_info_compatible_equal(info, *old_info)) {
- dassert(
- false,
- "conflict app info from (%s) for id(%d): new_info(%s), old_info(%s)",
- replica_nodes[i].to_string(),
- info.app_id,
- boost::lexical_cast<std::string>(info).c_str(),
- boost::lexical_cast<std::string>(*old_info).c_str());
- }
+ CHECK(app_info_compatible_equal(info, *old_info),
+ "conflict app info from ({}) for id({}): new_info({}), old_info({})",
+ replica_nodes[i].to_string(),
Review Comment:
```suggestion
replica_nodes[i],
```
##########
src/replica/mutation_log.cpp:
##########
@@ -788,23 +775,14 @@ error_code mutation_log::create_new_log_file()
"write mutation log file header failed, file = %s, err = %s",
logf->path().c_str(),
err.to_string());
- if (_io_error_callback) {
- _io_error_callback(err);
- } else {
- dassert(false, "unhandled error");
- }
+ CHECK(_io_error_callback, "");
+ _io_error_callback(err);
}
},
0);
- dassert(_global_end_offset ==
- _current_log_file->start_offset() + sizeof(log_block_header) + header_len,
- "%" PRId64 " VS %" PRId64 "(%" PRId64 " + %d + %d)",
- _global_end_offset,
- _current_log_file->start_offset() + sizeof(log_block_header) + header_len,
- _current_log_file->start_offset(),
- (int)sizeof(log_block_header),
- (int)header_len);
+ CHECK_EQ(_global_end_offset,
+ _current_log_file->start_offset() + sizeof(log_block_header) + header_len);
Review Comment:
Whether should we keep each addend in message to hint ?
##########
src/meta/server_state.cpp:
##########
@@ -1844,7 +1819,7 @@ void server_state::downgrade_stateless_nodes(std::shared_ptr<app_state> &app,
break;
}
}
- dassert(!req->node.is_invalid(), "invalid node address, address = %s", req->node.to_string());
+ CHECK(!req->node.is_invalid(), "invalid node address, address = {}", req->node.to_string());
Review Comment:
```suggestion
CHECK(!req->node.is_invalid(), "invalid node address, address = {}", req->node);
```
##########
src/meta/server_state.cpp:
##########
@@ -1695,12 +1669,12 @@ void server_state::recall_partition(std::shared_ptr<app_state> &app, int pidx)
server_state::sStateHash,
std::chrono::seconds(1));
} else {
- dassert(false, "unable to handle this(%s) right now", error.to_string());
+ CHECK(false, "unable to handle this({}) right now", error.to_string());
Review Comment:
```suggestion
CHECK(false, "unable to handle this({}) right now", error);
```
##########
src/meta/server_state.cpp:
##########
@@ -2693,7 +2649,7 @@ void server_state::do_update_app_info(const std::string &app_path,
0,
std::chrono::seconds(1));
} else {
- dassert(false, "we can't handle this, error(%s)", ec.to_string());
+ CHECK(false, "we can't handle this, error({})", ec.to_string());
Review Comment:
```suggestion
CHECK(false, "we can't handle this, error({})", ec);
```
##########
src/replica/replica_config.cpp:
##########
@@ -158,30 +155,16 @@ void replica::add_potential_secondary(configuration_update_request &proposal)
return;
}
- dassert(proposal.config.ballot == get_ballot(),
- "invalid ballot, %" PRId64 " VS %" PRId64 "",
- proposal.config.ballot,
- get_ballot());
- dassert(proposal.config.pid == _primary_states.membership.pid,
- "(%d.%d) VS (%d.%d)",
- proposal.config.pid.get_app_id(),
- proposal.config.pid.get_partition_index(),
- _primary_states.membership.pid.get_app_id(),
- _primary_states.membership.pid.get_partition_index());
- dassert(proposal.config.primary == _primary_states.membership.primary,
- "%s VS %s",
- proposal.config.primary.to_string(),
- _primary_states.membership.primary.to_string());
- dassert(proposal.config.secondaries == _primary_states.membership.secondaries,
- "count(%d) VS count(%d)",
- (int)proposal.config.secondaries.size(),
- (int)_primary_states.membership.secondaries.size());
- dassert(!_primary_states.check_exist(proposal.node, partition_status::PS_PRIMARY),
- "node = %s",
- proposal.node.to_string());
- dassert(!_primary_states.check_exist(proposal.node, partition_status::PS_SECONDARY),
- "node = %s",
- proposal.node.to_string());
+ CHECK_EQ(proposal.config.ballot, get_ballot());
+ CHECK_EQ(proposal.config.pid, _primary_states.membership.pid);
+ CHECK_EQ(proposal.config.primary, _primary_states.membership.primary);
+ CHECK(proposal.config.secondaries == _primary_states.membership.secondaries, "");
+ CHECK(!_primary_states.check_exist(proposal.node, partition_status::PS_PRIMARY),
+ "node = {}",
+ proposal.node.to_string());
+ CHECK(!_primary_states.check_exist(proposal.node, partition_status::PS_SECONDARY),
+ "node = {}",
+ proposal.node.to_string());
Review Comment:
```suggestion
proposal.node);
```
##########
src/meta/server_state.cpp:
##########
@@ -2606,55 +2577,40 @@ void server_state::get_cluster_balance_score(double &primary_stddev, double &tot
void server_state::check_consistency(const dsn::gpid &gpid)
{
auto iter = _all_apps.find(gpid.get_app_id());
- dassert(iter != _all_apps.end(),
- "invalid gpid(%d.%d)",
- gpid.get_app_id(),
- gpid.get_partition_index());
+ CHECK(iter != _all_apps.end(), "invalid gpid({})", gpid);
app_state &app = *(iter->second);
partition_configuration &config = app.partitions[gpid.get_partition_index()];
if (app.is_stateful) {
if (config.primary.is_invalid() == false) {
auto it = _nodes.find(config.primary);
- dassert(it != _nodes.end(),
- "invalid primary address, address = %s",
- config.primary.to_string());
- dassert(it->second.served_as(gpid) == partition_status::PS_PRIMARY,
- "node should serve as PS_PRIMARY, but status = %s",
- dsn::enum_to_string(it->second.served_as(gpid)));
-
- auto it2 =
- std::find(config.last_drops.begin(), config.last_drops.end(), config.primary);
- dassert(it2 == config.last_drops.end(),
- "primary shouldn't appear in last_drops, address = %s",
- config.primary.to_string());
+ CHECK(it != _nodes.end(),
+ "invalid primary address, address = {}",
+ config.primary.to_string());
Review Comment:
```suggestion
config.primary);
```
##########
src/meta/server_state.cpp:
##########
@@ -2606,55 +2577,40 @@ void server_state::get_cluster_balance_score(double &primary_stddev, double &tot
void server_state::check_consistency(const dsn::gpid &gpid)
{
auto iter = _all_apps.find(gpid.get_app_id());
- dassert(iter != _all_apps.end(),
- "invalid gpid(%d.%d)",
- gpid.get_app_id(),
- gpid.get_partition_index());
+ CHECK(iter != _all_apps.end(), "invalid gpid({})", gpid);
app_state &app = *(iter->second);
partition_configuration &config = app.partitions[gpid.get_partition_index()];
if (app.is_stateful) {
if (config.primary.is_invalid() == false) {
auto it = _nodes.find(config.primary);
- dassert(it != _nodes.end(),
- "invalid primary address, address = %s",
- config.primary.to_string());
- dassert(it->second.served_as(gpid) == partition_status::PS_PRIMARY,
- "node should serve as PS_PRIMARY, but status = %s",
- dsn::enum_to_string(it->second.served_as(gpid)));
-
- auto it2 =
- std::find(config.last_drops.begin(), config.last_drops.end(), config.primary);
- dassert(it2 == config.last_drops.end(),
- "primary shouldn't appear in last_drops, address = %s",
- config.primary.to_string());
+ CHECK(it != _nodes.end(),
+ "invalid primary address, address = {}",
+ config.primary.to_string());
+ CHECK_EQ(it->second.served_as(gpid), partition_status::PS_PRIMARY);
+ CHECK(std::find(config.last_drops.begin(), config.last_drops.end(), config.primary) ==
+ config.last_drops.end(),
+ "primary shouldn't appear in last_drops, address = {}",
+ config.primary.to_string());
Review Comment:
```suggestion
config.primary);
```
##########
src/replica/replica_check.cpp:
##########
@@ -223,7 +223,7 @@ void replica::on_group_check_reply(error_code err,
}
auto r = _primary_states.group_check_pending_replies.erase(req->node);
- dassert(r == 1, "invalid node address, address = %s", req->node.to_string());
+ CHECK_EQ_MSG(r, 1, "invalid node address, address = {}", req->node.to_string());
Review Comment:
```suggestion
CHECK_EQ_MSG(r, 1, "invalid node address, address = {}", req->node);
```
##########
src/replica/replica_stub.cpp:
##########
@@ -780,10 +779,11 @@ void replica_stub::initialize_fs_manager(std::vector<std::string> &data_dirs,
count++;
}
- dassert_f(available_dirs.size() > 0,
- "initialize fs manager failed, no available data directory");
- error_code err = _fs_manager.initialize(available_dirs, available_dir_tags, false);
- dassert_f(err == dsn::ERR_OK, "initialize fs manager failed, err({})", err);
+ CHECK_GT_MSG(
+ available_dirs.size(), 0, "initialize fs manager failed, no available data directory");
+ CHECK_EQ_MSG(_fs_manager.initialize(available_dirs, available_dir_tags, false),
Review Comment:
May be executed twice, see [src/utils/fmt_logging.h](https://github.com/apache/incubator-pegasus/pull/1217/files#diff-02a287c86805a1841e2cfaffdbab494ec1b4c01e7fb8be1e6bd965c59ee020e2)
##########
src/replica/replica_stub.cpp:
##########
@@ -649,21 +648,21 @@ void replica_stub::initialize(const replication_options &opts, bool clear /* = f
// restart log service
_log->close();
_log = nullptr;
- if (!utils::filesystem::remove_path(_options.slog_dir)) {
- dassert(false, "remove directory %s failed", _options.slog_dir.c_str());
- }
+ CHECK(utils::filesystem::remove_path(_options.slog_dir),
+ "remove directory {} failed",
+ _options.slog_dir);
_log = new mutation_log_shared(_options.slog_dir,
_options.log_shared_file_size_mb,
_options.log_shared_force_flush,
&_counter_shared_log_recent_write_size);
- auto lerr = _log->open(nullptr, [this](error_code err) { this->handle_log_failure(err); });
- dassert(lerr == ERR_OK, "restart log service must succeed");
+ CHECK_EQ_MSG(_log->open(nullptr, [this](error_code err) { this->handle_log_failure(err); }),
Review Comment:
May be executed twice, see [src/utils/fmt_logging.h](https://github.com/apache/incubator-pegasus/pull/1217/files#diff-02a287c86805a1841e2cfaffdbab494ec1b4c01e7fb8be1e6bd965c59ee020e2)
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org