acelyc111 commented on code in PR #1658:
URL:
https://github.com/apache/incubator-pegasus/pull/1658#discussion_r1401671098
##########
src/client/replication_ddl_client.cpp:
##########
@@ -490,8 +525,16 @@ dsn::error_code replication_ddl_client::list_nodes(
return resp.err;
}
- for (const dsn::replication::node_info &n : resp.infos) {
- nodes[n.address] = n.status;
+ for (dsn::replication::node_info &n : resp.infos) {
+ host_port hp;
+ if (n.__isset.hp_address) {
+ hp = n.hp_address;
+ LOG_ERROR("lgh: replica_server hp {}", hp);
Review Comment:
Remove the debug logs.
##########
run.sh:
##########
@@ -485,6 +486,19 @@ function run_test()
run_start_zk
fi
pushd ${BUILD_LATEST_DIR}/bin/${module}
+ local function_tests=(
+ backup_restore_test
+ recovery_test
Review Comment:
Keep alignment.
##########
src/client/partition_resolver_simple.cpp:
##########
@@ -411,20 +414,21 @@ void
partition_resolver_simple::handle_pending_requests(std::deque<request_conte
}
/*search in cache*/
-rpc_address partition_resolver_simple::get_address(const
partition_configuration &config) const
+host_port partition_resolver_simple::get_host_port(const
partition_configuration &config) const
{
+ auto pc = config;
Review Comment:
```suggestion
const auto &pc = config;
```
##########
src/failure_detector/failure_detector.cpp:
##########
@@ -415,33 +431,47 @@ bool
failure_detector::end_ping_internal(::dsn::error_code err, const beacon_ack
/*
* the caller of the end_ping_internal should lock necessarily!!!
*/
+ host_port hp_this_node, hp_primary_node;
+ if (ack.__isset.hp_this_node) {
+ hp_this_node = ack.hp_this_node;
+ } else {
+ hp_this_node = host_port(ack.this_node);
+ }
+ if (ack.__isset.hp_primary_node) {
+ hp_primary_node = ack.hp_primary_node;
+ } else {
+ hp_primary_node = host_port(ack.primary_node);
+ }
+
uint64_t beacon_send_time = ack.time;
- auto node = ack.this_node;
if (err != ERR_OK) {
LOG_WARNING("ping master({}) failed, timeout_ms = {}, err = {}",
- node,
+ hp_this_node,
_beacon_timeout_milliseconds,
err);
_recent_beacon_fail_count->increment();
}
- master_map::iterator itr = _masters.find(node);
+ master_map::iterator itr = _masters.find(hp_this_node);
if (itr == _masters.end()) {
LOG_WARNING("received beacon ack without corresponding master, ignore
it, "
- "remote_master[{}], local_worker[{}]",
- node,
+ "remote_master[{}({})], local_worker[{}]",
+ hp_this_node,
+ ack.this_node,
dsn_primary_address());
Review Comment:
Also logging host_port by dsn_primary_host_port().
Other places are the same.
##########
src/meta/cluster_balance_policy.cpp:
##########
@@ -544,10 +545,21 @@ bool cluster_balance_policy::apply_move(const move_info
&move,
// add into migration list and selected_pid
partition_configuration pc;
pc.pid = move.pid;
- pc.primary = primary_addr;
- list[move.pid] = generate_balancer_request(*_global_view->apps, pc,
move.type, source, target);
+ pc.hp_primary = primary_hp;
+ std::shared_ptr<dsn::dns_resolver> resolver;
+ if (_svc == nullptr) {
+ resolver = std::make_shared<dsn::dns_resolver>();
Review Comment:
In which case the `_svc` is nullptr?
##########
src/meta/greedy_load_balancer.cpp:
##########
@@ -163,7 +163,7 @@ void greedy_load_balancer::score(meta_view view, double
&primary_stddev, double
bool greedy_load_balancer::all_replica_infos_collected(const node_state &ns)
{
- dsn::rpc_address n = ns.addr();
+ auto n = ns.host_port();
Review Comment:
```suggestion
const auto &n = ns.host_port();
```
##########
src/common/json_helper.h:
##########
@@ -423,6 +423,21 @@ inline bool json_decode(const dsn::json::JsonObject &in,
dsn::rpc_address &addre
return address.from_string_ipv4(rpc_address_string.c_str());
}
+// json serialization for rpc address, we use the string representation of a
address
+inline void json_encode(JsonWriter &out, const dsn::host_port &hp)
+{
+ json_encode(out, hp.to_string());
+}
+inline bool json_decode(const dsn::json::JsonObject &in, dsn::host_port &hp)
Review Comment:
Add some unit tests for the two functions.
##########
src/common/replication_common.cpp:
##########
@@ -164,6 +167,18 @@ int32_t
replication_options::app_mutation_2pc_min_replica_count(int32_t app_max_
}
}
+/*static*/ bool replica_helper::remove_node(::dsn::host_port node,
+ /*inout*/
std::vector<::dsn::host_port> &nodeList)
Review Comment:
```suggestion
/*inout*/
std::vector<::dsn::host_port> &nodes)
```
##########
src/client/replication_ddl_client.cpp:
##########
@@ -81,18 +82,51 @@ namespace replication {
using tp_output_format = ::dsn::utils::table_printer::output_format;
-replication_ddl_client::replication_ddl_client(const
std::vector<dsn::rpc_address> &meta_servers)
+replication_ddl_client::replication_ddl_client(const
std::vector<dsn::host_port> &meta_servers)
+ : _dns_resolver(new dns_resolver())
{
_meta_server.assign_group("meta-servers");
for (const auto &m : meta_servers) {
- if (!_meta_server.group_address()->add(m)) {
+ if (!_meta_server.group_host_port()->add(m)) {
LOG_WARNING("duplicate adress {}", m);
}
}
}
replication_ddl_client::~replication_ddl_client() {
_tracker.cancel_outstanding_tasks(); }
+void replication_ddl_client::set_meta_servers_leader()
Review Comment:
Sorry I didn't get your point, add some comment for this newly added
function (in header file) may help to comprehend.
##########
src/client/partition_resolver.cpp:
##########
@@ -124,7 +127,7 @@ void partition_resolver::call_task(const
rpc_response_task_ptr &t)
}
hdr.gpid = result.pid;
}
- dsn_rpc_call(result.address, t.get());
+ dsn_rpc_call(this->_dns_resolver->resolve_address(result.hp),
t.get());
Review Comment:
There will be performance degradation if resolving host name everytime,
fortunately, you have added a cache for that. Even though, it's worse than
using ip address directly.
A compromised way is to add an option to decide whether to resolve host name
every time or just once, and make resolving once as default.
IMO, it's acceptable to resolve host name just once, because if a host
changes it's ip address, it's reasonable to ask the administrators to restart
the Pegasus servers and clients. If they don't accept this, they should know
and accept the performance degradation caused by resolving host name every time.
##########
src/failure_detector/failure_detector.cpp:
##########
@@ -352,33 +355,46 @@ std::string failure_detector::get_allow_list(const
std::vector<std::string> &arg
void failure_detector::on_ping_internal(const beacon_msg &beacon, /*out*/
beacon_ack &ack)
{
+ host_port hp_from_addr, hp_to_addr;
+ if (beacon.__isset.hp_from_addr) {
Review Comment:
Add a macro like this to simplify code:
```
#define HOST_PORT(obj, field, target) \
do { \
if (obj.__isset.hp_##field) { \
target = obj.hp_##field; \
} else { \
target = std::move(host_port(obj.field)); \
} \
} while (0)
```
--
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]
