Skip to content

Commit b72274f

Browse files
authored
refactor: Simplify code on partition_configuration (#2051)
There is no functional changes, but only refactoring, includes: - Use range-based loop - Unify the naming of partition_configuration variables to `pc` (or `pcs`) - Use `fmt::join` for simplify container formating - Reduce some if-else statements depth This patch dosen't change the names in thrift IDL which leads more changes on client drivers, we can do that in following patches.
1 parent a128565 commit b72274f

File tree

80 files changed

+1086
-1152
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

80 files changed

+1086
-1152
lines changed

src/client/partition_resolver_simple.cpp

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -302,26 +302,24 @@ void partition_resolver_simple::query_config_reply(error_code err,
302302
_app_partition_count = resp.partition_count;
303303
_app_is_stateful = resp.is_stateful;
304304

305-
for (auto it = resp.partitions.begin(); it != resp.partitions.end(); ++it) {
306-
auto &new_config = *it;
307-
305+
for (const auto &new_pc : resp.partitions) {
308306
LOG_DEBUG_PREFIX("query config reply, gpid = {}, ballot = {}, primary = {}",
309-
new_config.pid,
310-
new_config.ballot,
311-
FMT_HOST_PORT_AND_IP(new_config, primary));
307+
new_pc.pid,
308+
new_pc.ballot,
309+
FMT_HOST_PORT_AND_IP(new_pc, primary));
312310

313-
auto it2 = _config_cache.find(new_config.pid.get_partition_index());
311+
auto it2 = _config_cache.find(new_pc.pid.get_partition_index());
314312
if (it2 == _config_cache.end()) {
315-
std::unique_ptr<partition_info> pi(new partition_info);
313+
auto pi = std::make_unique<partition_info>();
316314
pi->timeout_count = 0;
317-
pi->config = new_config;
318-
_config_cache.emplace(new_config.pid.get_partition_index(), std::move(pi));
319-
} else if (_app_is_stateful && it2->second->config.ballot < new_config.ballot) {
315+
pi->pc = new_pc;
316+
_config_cache.emplace(new_pc.pid.get_partition_index(), std::move(pi));
317+
} else if (_app_is_stateful && it2->second->pc.ballot < new_pc.ballot) {
320318
it2->second->timeout_count = 0;
321-
it2->second->config = new_config;
319+
it2->second->pc = new_pc;
322320
} else if (!_app_is_stateful) {
323321
it2->second->timeout_count = 0;
324-
it2->second->config = new_config;
322+
it2->second->pc = new_pc;
325323
} else {
326324
// nothing to do
327325
}
@@ -413,32 +411,30 @@ void partition_resolver_simple::handle_pending_requests(std::deque<request_conte
413411
}
414412

415413
/*search in cache*/
416-
host_port partition_resolver_simple::get_host_port(const partition_configuration &config) const
414+
host_port partition_resolver_simple::get_host_port(const partition_configuration &pc) const
417415
{
418416
if (_app_is_stateful) {
419-
return config.hp_primary;
417+
return pc.hp_primary;
420418
}
421419

422-
if (config.hp_last_drops.empty()) {
420+
if (pc.hp_last_drops.empty()) {
423421
return host_port();
424422
}
425423

426-
return config.hp_last_drops[rand::next_u32(0, config.last_drops.size() - 1)];
424+
return pc.hp_last_drops[rand::next_u32(0, pc.last_drops.size() - 1)];
427425
}
428426

429427
error_code partition_resolver_simple::get_host_port(int partition_index, /*out*/ host_port &hp)
430428
{
431-
// partition_configuration config;
432429
{
433430
zauto_read_lock l(_config_lock);
434431
auto it = _config_cache.find(partition_index);
435432
if (it != _config_cache.end()) {
436-
// config = it->second->config;
437-
if (it->second->config.ballot < 0) {
433+
if (it->second->pc.ballot < 0) {
438434
// client query config for splitting app, child partition is not ready
439435
return ERR_CHILD_NOT_READY;
440436
}
441-
hp = get_host_port(it->second->config);
437+
hp = get_host_port(it->second->pc);
442438
if (!hp) {
443439
return ERR_IO_PENDING;
444440
} else {

src/client/partition_resolver_simple.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ class partition_resolver_simple : public partition_resolver
6565
struct partition_info
6666
{
6767
int timeout_count;
68-
::dsn::partition_configuration config;
68+
::dsn::partition_configuration pc;
6969
};
7070
mutable dsn::zrwlock_nr _config_lock;
7171
std::unordered_map<int, std::unique_ptr<partition_info>> _config_cache;
@@ -107,7 +107,7 @@ class partition_resolver_simple : public partition_resolver
107107

108108
private:
109109
// local routines
110-
host_port get_host_port(const partition_configuration &config) const;
110+
host_port get_host_port(const partition_configuration &pc) const;
111111
error_code get_host_port(int partition_index, /*out*/ host_port &hp);
112112
void handle_pending_requests(std::deque<request_context_ptr> &reqs, error_code err);
113113
void clear_all_pending_requests();

src/client/replication_ddl_client.cpp

Lines changed: 41 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ dsn::error_code replication_ddl_client::wait_app_ready(const std::string &app_na
166166
CHECK_EQ(partition_count, query_resp.partition_count);
167167
int ready_count = 0;
168168
for (int i = 0; i < partition_count; i++) {
169-
const partition_configuration &pc = query_resp.partitions[i];
169+
const auto &pc = query_resp.partitions[i];
170170
if (pc.hp_primary && (pc.hp_secondaries.size() + 1 >= max_replica_count)) {
171171
ready_count++;
172172
}
@@ -422,8 +422,8 @@ dsn::error_code replication_ddl_client::list_apps(const dsn::app_status::type st
422422
}
423423
int32_t app_id;
424424
int32_t partition_count;
425-
std::vector<partition_configuration> partitions;
426-
r = list_app(info.app_name, app_id, partition_count, partitions);
425+
std::vector<partition_configuration> pcs;
426+
r = list_app(info.app_name, app_id, partition_count, pcs);
427427
if (r != dsn::ERR_OK) {
428428
LOG_ERROR("list app({}) failed, err = {}", info.app_name, r);
429429
return r;
@@ -433,18 +433,18 @@ dsn::error_code replication_ddl_client::list_apps(const dsn::app_status::type st
433433
int fully_healthy = 0;
434434
int write_unhealthy = 0;
435435
int read_unhealthy = 0;
436-
for (int i = 0; i < partitions.size(); i++) {
437-
const dsn::partition_configuration &p = partitions[i];
436+
for (const auto &pc : pcs) {
438437
int replica_count = 0;
439-
if (p.hp_primary) {
438+
if (pc.hp_primary) {
440439
replica_count++;
441440
}
442-
replica_count += p.hp_secondaries.size();
443-
if (p.hp_primary) {
444-
if (replica_count >= p.max_replica_count)
441+
replica_count += pc.hp_secondaries.size();
442+
if (pc.hp_primary) {
443+
if (replica_count >= pc.max_replica_count) {
445444
fully_healthy++;
446-
else if (replica_count < 2)
445+
} else if (replica_count < 2) {
447446
write_unhealthy++;
447+
}
448448
} else {
449449
write_unhealthy++;
450450
read_unhealthy++;
@@ -566,22 +566,21 @@ dsn::error_code replication_ddl_client::list_nodes(const dsn::replication::node_
566566
for (auto &app : apps) {
567567
int32_t app_id;
568568
int32_t partition_count;
569-
std::vector<partition_configuration> partitions;
570-
r = list_app(app.app_name, app_id, partition_count, partitions);
569+
std::vector<partition_configuration> pcs;
570+
r = list_app(app.app_name, app_id, partition_count, pcs);
571571
if (r != dsn::ERR_OK) {
572572
return r;
573573
}
574574

575-
for (int i = 0; i < partitions.size(); i++) {
576-
const dsn::partition_configuration &p = partitions[i];
577-
if (p.hp_primary) {
578-
auto find = tmp_map.find(p.hp_primary);
575+
for (const auto &pc : pcs) {
576+
if (pc.hp_primary) {
577+
auto find = tmp_map.find(pc.hp_primary);
579578
if (find != tmp_map.end()) {
580579
find->second.primary_count++;
581580
}
582581
}
583-
for (int j = 0; j < p.hp_secondaries.size(); j++) {
584-
auto find = tmp_map.find(p.hp_secondaries[j]);
582+
for (const auto &secondary : pc.hp_secondaries) {
583+
auto find = tmp_map.find(secondary);
585584
if (find != tmp_map.end()) {
586585
find->second.secondary_count++;
587586
}
@@ -723,13 +722,13 @@ dsn::error_code replication_ddl_client::list_app(const std::string &app_name,
723722
int32_t app_id = 0;
724723
int32_t partition_count = 0;
725724
int32_t max_replica_count = 0;
726-
std::vector<partition_configuration> partitions;
727-
dsn::error_code err = list_app(app_name, app_id, partition_count, partitions);
725+
std::vector<partition_configuration> pcs;
726+
dsn::error_code err = list_app(app_name, app_id, partition_count, pcs);
728727
if (err != dsn::ERR_OK) {
729728
return err;
730729
}
731-
if (!partitions.empty()) {
732-
max_replica_count = partitions[0].max_replica_count;
730+
if (!pcs.empty()) {
731+
max_replica_count = pcs[0].max_replica_count;
733732
}
734733

735734
// print query_cfg_response
@@ -765,41 +764,33 @@ dsn::error_code replication_ddl_client::list_app(const std::string &app_name,
765764
int fully_healthy = 0;
766765
int write_unhealthy = 0;
767766
int read_unhealthy = 0;
768-
for (const auto &p : partitions) {
767+
for (const auto &pc : pcs) {
769768
int replica_count = 0;
770-
if (p.hp_primary) {
769+
if (pc.hp_primary) {
771770
replica_count++;
772-
node_stat[p.hp_primary].first++;
771+
node_stat[pc.hp_primary].first++;
773772
total_prim_count++;
774773
}
775-
replica_count += p.hp_secondaries.size();
776-
total_sec_count += p.hp_secondaries.size();
777-
if (p.hp_primary) {
778-
if (replica_count >= p.max_replica_count)
774+
replica_count += pc.hp_secondaries.size();
775+
total_sec_count += pc.hp_secondaries.size();
776+
if (pc.hp_primary) {
777+
if (replica_count >= pc.max_replica_count) {
779778
fully_healthy++;
780-
else if (replica_count < 2)
779+
} else if (replica_count < 2) {
781780
write_unhealthy++;
781+
}
782782
} else {
783783
write_unhealthy++;
784784
read_unhealthy++;
785785
}
786-
tp_details.add_row(p.pid.get_partition_index());
787-
tp_details.append_data(p.ballot);
788-
std::stringstream oss;
789-
oss << replica_count << "/" << p.max_replica_count;
790-
tp_details.append_data(oss.str());
791-
tp_details.append_data(p.hp_primary ? p.hp_primary.to_string() : "-");
792-
oss.str("");
793-
oss << "[";
794-
// TODO (yingchun) join
795-
for (int j = 0; j < p.hp_secondaries.size(); j++) {
796-
if (j != 0)
797-
oss << ",";
798-
oss << p.hp_secondaries[j];
799-
node_stat[p.hp_secondaries[j]].second++;
786+
for (const auto &secondary : pc.hp_secondaries) {
787+
node_stat[secondary].second++;
800788
}
801-
oss << "]";
802-
tp_details.append_data(oss.str());
789+
tp_details.add_row(pc.pid.get_partition_index());
790+
tp_details.append_data(pc.ballot);
791+
tp_details.append_data(fmt::format("{}/{}", replica_count, pc.max_replica_count));
792+
tp_details.append_data(pc.hp_primary ? pc.hp_primary.to_string() : "-");
793+
tp_details.append_data(fmt::format("[{}]", fmt::join(pc.hp_secondaries, ",")));
803794
}
804795
mtp.add(std::move(tp_details));
805796

@@ -837,7 +828,7 @@ dsn::error_code replication_ddl_client::list_app(const std::string &app_name,
837828
dsn::error_code replication_ddl_client::list_app(const std::string &app_name,
838829
int32_t &app_id,
839830
int32_t &partition_count,
840-
std::vector<partition_configuration> &partitions)
831+
std::vector<partition_configuration> &pcs)
841832
{
842833
RETURN_EC_NOT_OK_MSG(validate_app_name(app_name), "invalid app_name: '{}'", app_name);
843834

@@ -859,7 +850,7 @@ dsn::error_code replication_ddl_client::list_app(const std::string &app_name,
859850

860851
app_id = resp.app_id;
861852
partition_count = resp.partition_count;
862-
partitions = resp.partitions;
853+
pcs = resp.partitions;
863854

864855
return dsn::ERR_OK;
865856
}
@@ -1322,8 +1313,8 @@ dsn::error_code replication_ddl_client::query_restore(int32_t restore_app_id, bo
13221313
::dsn::unmarshall(resp_task->get_response(), response);
13231314
if (response.err == ERR_OK) {
13241315
int overall_progress = 0;
1325-
for (const auto &p : response.restore_progress) {
1326-
overall_progress += p;
1316+
for (const auto &progress : response.restore_progress) {
1317+
overall_progress += progress;
13271318
}
13281319
overall_progress = overall_progress / response.restore_progress.size();
13291320
overall_progress = overall_progress / 10;

src/client/replication_ddl_client.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ class replication_ddl_client
125125
dsn::error_code list_app(const std::string &app_name,
126126
int32_t &app_id,
127127
int32_t &partition_count,
128-
std::vector<partition_configuration> &partitions);
128+
std::vector<partition_configuration> &pcs);
129129

130130
dsn::replication::configuration_meta_control_response
131131
control_meta_function_level(meta_function_level::type level);

src/common/json_helper.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -448,8 +448,8 @@ inline bool json_decode(const dsn::json::JsonObject &in, dsn::host_port &hp)
448448
return static_cast<bool>(hp);
449449
}
450450

451-
inline void json_encode(JsonWriter &out, const dsn::partition_configuration &config);
452-
inline bool json_decode(const JsonObject &in, dsn::partition_configuration &config);
451+
inline void json_encode(JsonWriter &out, const dsn::partition_configuration &pc);
452+
inline bool json_decode(const JsonObject &in, dsn::partition_configuration &pc);
453453
inline void json_encode(JsonWriter &out, const dsn::app_info &info);
454454
inline bool json_decode(const JsonObject &in, dsn::app_info &info);
455455
inline void json_encode(JsonWriter &out, const dsn::replication::file_meta &f_meta);

src/common/replication_common.cpp

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -166,26 +166,26 @@ int32_t replication_options::app_mutation_2pc_min_replica_count(int32_t app_max_
166166
}
167167
}
168168

169-
/*static*/ bool replica_helper::get_replica_config(const partition_configuration &partition_config,
169+
/*static*/ bool replica_helper::get_replica_config(const partition_configuration &pc,
170170
const ::dsn::host_port &node,
171-
/*out*/ replica_configuration &replica_config)
171+
/*out*/ replica_configuration &rc)
172172
{
173-
replica_config.pid = partition_config.pid;
174-
replica_config.ballot = partition_config.ballot;
175-
replica_config.learner_signature = invalid_signature;
176-
SET_OBJ_IP_AND_HOST_PORT(replica_config, primary, partition_config, primary);
173+
rc.pid = pc.pid;
174+
rc.ballot = pc.ballot;
175+
rc.learner_signature = invalid_signature;
176+
SET_OBJ_IP_AND_HOST_PORT(rc, primary, pc, primary);
177177

178-
if (node == partition_config.hp_primary) {
179-
replica_config.status = partition_status::PS_PRIMARY;
178+
if (node == pc.hp_primary) {
179+
rc.status = partition_status::PS_PRIMARY;
180180
return true;
181181
}
182182

183-
if (utils::contains(partition_config.hp_secondaries, node)) {
184-
replica_config.status = partition_status::PS_SECONDARY;
183+
if (utils::contains(pc.hp_secondaries, node)) {
184+
rc.status = partition_status::PS_SECONDARY;
185185
return true;
186186
}
187187

188-
replica_config.status = partition_status::PS_INACTIVE;
188+
rc.status = partition_status::PS_INACTIVE;
189189
return false;
190190
}
191191

src/common/replication_other_types.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,8 @@ inline bool is_partition_config_equal(const partition_configuration &pc1,
7979
const partition_configuration &pc2)
8080
{
8181
// secondaries no need to be same order
82-
for (const auto &hp : pc1.hp_secondaries) {
83-
if (!is_secondary(pc2, hp)) {
82+
for (const auto &pc1_secondary : pc1.hp_secondaries) {
83+
if (!is_secondary(pc2, pc1_secondary)) {
8484
return false;
8585
}
8686
}
@@ -106,9 +106,9 @@ class replica_helper
106106
}
107107
return false;
108108
}
109-
static bool get_replica_config(const partition_configuration &partition_config,
109+
static bool get_replica_config(const partition_configuration &pc,
110110
const ::dsn::host_port &node,
111-
/*out*/ replica_configuration &replica_config);
111+
/*out*/ replica_configuration &rc);
112112

113113
// Return true if 'server_list' is a valid comma-separated list of servers, otherwise return
114114
// false. The result is filled into 'servers' if success.

src/meta/backup_engine.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ void backup_engine::backup_app_partition(const gpid &pid)
182182
_is_backup_failed = true;
183183
return;
184184
}
185-
partition_primary = app->partitions[pid.get_partition_index()].hp_primary;
185+
partition_primary = app->pcs[pid.get_partition_index()].hp_primary;
186186
}
187187

188188
if (!partition_primary) {

0 commit comments

Comments
 (0)