empiredan commented on code in PR #1825:
URL:
https://github.com/apache/incubator-pegasus/pull/1825#discussion_r1445960892
##########
src/runtime/rpc/rpc_host_port.h:
##########
@@ -85,6 +86,17 @@ class host_port
// This function is used for validating the format of string like
"localhost:8888".
bool from_string(const std::string &s);
+ // For pring log use only, get string better use 'to_std_string'
+ // using get_char_str by mistake may lead to some bugs:
Review Comment:
```suggestion
// Used only for printing log, such as log_prefix(). Better to use
// to_std_string(), since using get_char_str by mistake may lead
// to some bugs, for example:
```
##########
src/runtime/test/host_port_test.cpp:
##########
@@ -204,27 +244,26 @@ TEST(host_port_test, dns_resolver)
{
host_port hp_grp;
hp_grp.assign_group("test_group");
- rpc_group_host_port *g = hp_grp.group_host_port();
+ auto g_hp = hp_grp.group_host_port();
host_port hp1("localhost", 8080);
- ASSERT_TRUE(g->add(hp1));
+ ASSERT_TRUE(g_hp->add(hp1));
host_port hp2("localhost", 8081);
- g->set_leader(hp2);
+ g_hp->set_leader(hp2);
auto addr_grp = resolver.resolve_address(hp_grp);
+ auto g_addr = addr_grp.group_address();
- ASSERT_EQ(addr_grp.group_address()->is_update_leader_automatically(),
- hp_grp.group_host_port()->is_update_leader_automatically());
- ASSERT_EQ(strcmp(addr_grp.group_address()->name(),
hp_grp.group_host_port()->name()), 0);
- ASSERT_EQ(addr_grp.group_address()->count(),
hp_grp.group_host_port()->count());
- ASSERT_EQ(host_port(addr_grp.group_address()->leader()),
- hp_grp.group_host_port()->leader());
+ ASSERT_EQ(g_addr->is_update_leader_automatically(),
g_hp->is_update_leader_automatically());
+ ASSERT_STREQ(g_addr->name(), g_hp->name());
+ ASSERT_EQ(g_addr->count(), g_hp->count());
+ ASSERT_EQ(host_port(g_addr->leader()), g_hp->leader());
}
}
void send_and_check_host_port_by_serialize(const host_port &hp,
dsn_msg_serialize_format t)
{
- auto hp_str = hp.to_string();
+ auto hp_str = hp.to_std_string();
Review Comment:
```suggestion
const auto &hp_str = hp.to_std_string();
```
##########
src/runtime/test/host_port_test.cpp:
##########
@@ -111,8 +116,8 @@ TEST(host_port_test, rpc_group_host_port)
host_port hp_grp;
hp_grp.assign_group("test_group");
ASSERT_EQ(HOST_TYPE_GROUP, hp_grp.type());
- rpc_group_host_port *g = hp_grp.group_host_port();
- ASSERT_EQ(std::string("test_group"), g->name());
+ auto g = hp_grp.group_host_port();
Review Comment:
```suggestion
const auto &g = hp_grp.group_host_port();
```
##########
src/runtime/rpc/rpc_host_port.h:
##########
@@ -85,6 +86,17 @@ class host_port
// This function is used for validating the format of string like
"localhost:8888".
bool from_string(const std::string &s);
+ // For pring log use only, get string better use 'to_std_string'
+ // using get_char_str by mistake may lead to some bugs:
+ // // The size of hosts are more than 8.
Review Comment:
```suggestion
// The size of hosts are more than 8.
```
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -151,25 +151,35 @@ host_port &host_port::operator=(const host_port &other)
return *this;
}
-std::string host_port::to_string() const
+std::string host_port::to_std_string() const
{
switch (type()) {
case HOST_TYPE_IPV4:
return fmt::format("{}:{}", _host, _port);
case HOST_TYPE_GROUP:
- return fmt::format("address group {}", group_host_port()->name());
+ return fmt::format("host_port group {}", group_host_port()->name());
default:
- return "invalid address";
+ return "invalid host_port";
}
}
+// hostname has a maximum of 253 ASCII characters
+// uint_16 port maximum is 65535
+// 253 + 1(colon) + 5 = 259
+// align the memory address, 256 < 259 < 320
Review Comment:
```suggestion
// A host name has 253 ASCII characters at most. The max length of
// port(uint16_t, 65535) is 5. Thus the max length of "<host>:<port>"
// is 253 + 1(colon) + 5 = 259.
//
// Set the size to 64 * 5 = 320 to be 64-bytes aligned.
```
--
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]
