This is an automated email from the ASF dual-hosted git repository.
laiyingchun pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pegasus.git
The following commit(s) were added to refs/heads/master by this push:
new 1d277d9f2 refactor(host_port): Make some functions as static (#1894)
1d277d9f2 is described below
commit 1d277d9f294c139d7d340b33205d1894e9e1676c
Author: Yingchun Lai <[email protected]>
AuthorDate: Mon Feb 5 14:37:32 2024 +0800
refactor(host_port): Make some functions as static (#1894)
This patch mainly has cchanges:
- Change the constructor `host_port(rpc_address addr)` to a static function
`host_port from_address(rpc_address addr)`. It's a good practice to keep
constructors light and fast, this function contains a reverse DNS lookup,
it may be slow.
- Change `bool from_string(const std::string &s)` to a static function
`host_port from_string(const std::string &host_port_str)` in host_port.
It seems there isn't a scenario to modify an exist host_port object by
calling obj.from_string(s), so make it as static to clarify this.
This patch also adds SCOPED_LOG_SLOW_EXECUTION to log slow operations if it
exceed 100 milliseconds.
---
src/common/json_helper.h | 3 ++-
src/runtime/rpc/group_host_port.h | 4 ++--
src/runtime/rpc/rpc_host_port.cpp | 47 +++++++++++++++++++++++--------------
src/runtime/rpc/rpc_host_port.h | 14 +++++++----
src/runtime/test/host_port_test.cpp | 10 ++++----
src/utils/test/json_helper_test.cpp | 2 +-
6 files changed, 50 insertions(+), 30 deletions(-)
diff --git a/src/common/json_helper.h b/src/common/json_helper.h
index 291909bc8..72d982f2f 100644
--- a/src/common/json_helper.h
+++ b/src/common/json_helper.h
@@ -436,7 +436,8 @@ inline bool json_decode(const dsn::json::JsonObject &in,
dsn::host_port &hp)
if (host_port_string == "invalid host_port") {
return true;
}
- return hp.from_string(host_port_string);
+ hp = host_port::from_string(host_port_string);
+ return !hp.is_invalid();
}
inline void json_encode(JsonWriter &out, const dsn::partition_configuration
&config);
diff --git a/src/runtime/rpc/group_host_port.h
b/src/runtime/rpc/group_host_port.h
index 1bc8989ea..a92937c75 100644
--- a/src/runtime/rpc/group_host_port.h
+++ b/src/runtime/rpc/group_host_port.h
@@ -128,10 +128,10 @@ inline rpc_group_host_port::rpc_group_host_port(const
rpc_group_address *g_addr)
{
_name = g_addr->name();
for (const auto &addr : g_addr->members()) {
- CHECK_TRUE(add(host_port(addr)));
+ CHECK_TRUE(add(host_port::from_address(addr)));
}
_update_leader_automatically = g_addr->is_update_leader_automatically();
- set_leader(host_port(g_addr->leader()));
+ set_leader(host_port::from_address(g_addr->leader()));
}
inline rpc_group_host_port &rpc_group_host_port::operator=(const
rpc_group_host_port &other)
diff --git a/src/runtime/rpc/rpc_host_port.cpp
b/src/runtime/rpc/rpc_host_port.cpp
index 359cf5a3b..a00265adc 100644
--- a/src/runtime/rpc/rpc_host_port.cpp
+++ b/src/runtime/rpc/rpc_host_port.cpp
@@ -28,9 +28,11 @@
#include "fmt/core.h"
#include "runtime/rpc/group_host_port.h"
#include "runtime/rpc/rpc_host_port.h"
+#include "utils/api_utilities.h"
#include "utils/error_code.h"
#include "utils/ports.h"
#include "utils/string_conv.h"
+#include "utils/timer.h"
#include "utils/utils.h"
namespace dsn {
@@ -40,46 +42,57 @@ const host_port host_port::s_invalid_host_port;
host_port::host_port(std::string host, uint16_t port)
: _host(std::move(host)), _port(port), _type(HOST_TYPE_IPV4)
{
- CHECK_NE_MSG(rpc_address::ipv4_from_host(_host.c_str()), 0, "invalid
hostname: {}", _host);
+ // ipv4_from_host may be slow, just call it in DEBUG version.
+ DCHECK_NE_MSG(rpc_address::ipv4_from_host(_host.c_str()), 0, "invalid
hostname: {}", _host);
}
-host_port::host_port(rpc_address addr)
+host_port host_port::from_address(rpc_address addr)
{
+ host_port hp;
+ SCOPED_LOG_SLOW_EXECUTION(
+ WARNING, 100, "construct host_port '{}' from rpc_address '{}'", hp,
addr);
switch (addr.type()) {
case HOST_TYPE_IPV4: {
- CHECK(utils::hostname_from_ip(htonl(addr.ip()), &_host),
+ CHECK(utils::hostname_from_ip(htonl(addr.ip()), &hp._host),
"invalid host_port {}",
addr.ipv4_str());
- _port = addr.port();
+ hp._port = addr.port();
} break;
case HOST_TYPE_GROUP: {
- _group_host_port =
std::make_shared<rpc_group_host_port>(addr.group_address());
+ hp._group_host_port =
std::make_shared<rpc_group_host_port>(addr.group_address());
} break;
default:
break;
}
- _type = addr.type();
+
+ // Now is_invalid() return false.
+ hp._type = addr.type();
+ return hp;
}
-bool host_port::from_string(const std::string &s)
+host_port host_port::from_string(const std::string &host_port_str)
{
- const auto pos = s.find_last_of(':');
+ host_port hp;
+ SCOPED_LOG_SLOW_EXECUTION(
+ WARNING, 100, "construct host_port '{}' from string '{}'", hp,
host_port_str);
+ const auto pos = host_port_str.find_last_of(':');
if (dsn_unlikely(pos == std::string::npos)) {
- return false;
+ return hp;
}
- _host = s.substr(0, pos);
- std::string port = s.substr(pos + 1);
- if (dsn_unlikely(!dsn::buf2uint16(port, _port))) {
- return false;
+ hp._host = host_port_str.substr(0, pos);
+ const auto port_str = host_port_str.substr(pos + 1);
+ if (dsn_unlikely(!dsn::buf2uint16(port_str, hp._port))) {
+ return hp;
}
- if (dsn_unlikely(rpc_address::ipv4_from_host(_host.c_str()) == 0)) {
- return false;
+ if (dsn_unlikely(rpc_address::ipv4_from_host(hp._host.c_str()) == 0)) {
+ return hp;
}
- _type = HOST_TYPE_IPV4;
- return true;
+ // Now is_invalid() return false.
+ hp._type = HOST_TYPE_IPV4;
+ return hp;
}
void host_port::reset()
diff --git a/src/runtime/rpc/rpc_host_port.h b/src/runtime/rpc/rpc_host_port.h
index 4cda4180b..cd8df1fe6 100644
--- a/src/runtime/rpc/rpc_host_port.h
+++ b/src/runtime/rpc/rpc_host_port.h
@@ -52,7 +52,6 @@ public:
static const host_port s_invalid_host_port;
explicit host_port() = default;
explicit host_port(std::string host, uint16_t port);
- explicit host_port(rpc_address addr);
host_port(const host_port &other) { *this = other; }
host_port &operator=(const host_port &other);
@@ -64,7 +63,7 @@ public:
const std::string &host() const { return _host; }
uint16_t port() const { return _port; }
- bool is_invalid() const { return _type == HOST_TYPE_INVALID; }
+ [[nodiscard]] bool is_invalid() const { return _type == HOST_TYPE_INVALID;
}
std::string to_string() const;
@@ -83,8 +82,15 @@ public:
// Resolve host_port to rpc_addresses.
// Trere may be multiple rpc_addresses for one host_port.
error_s resolve_addresses(std::vector<rpc_address> &addresses) const;
- // This function is used for validating the format of string like
"localhost:8888".
- bool from_string(const std::string &s);
+
+ // Construct a host_port object from 'addr'
+ static host_port from_address(rpc_address addr);
+
+ // Construct a host_port object from 'host_port_str', the latter is in the
format of
+ // "localhost:8888".
+ // NOTE: The constructed host_port object maybe invalid, remember to check
it by is_invalid()
+ // before using it.
+ static host_port from_string(const std::string &host_port_str);
// for serialization in thrift format
uint32_t read(::apache::thrift::protocol::TProtocol *iprot);
diff --git a/src/runtime/test/host_port_test.cpp
b/src/runtime/test/host_port_test.cpp
index 05d6982b2..2f7dd24a7 100644
--- a/src/runtime/test/host_port_test.cpp
+++ b/src/runtime/test/host_port_test.cpp
@@ -63,7 +63,7 @@ TEST(host_port_test, host_port_build)
{
rpc_address addr = rpc_address("localhost", 8080);
- host_port hp1(addr);
+ host_port hp1 = host_port::from_address(addr);
ASSERT_EQ(hp, hp1);
}
}
@@ -91,14 +91,14 @@ TEST(host_port_test, operators)
std::string hp_str = "localhost:8080";
host_port hp3;
ASSERT_TRUE(hp3.is_invalid());
- ASSERT_TRUE(hp3.from_string(hp_str));
+ hp3 = host_port::from_string(hp_str);
ASSERT_EQ(hp, hp3);
ASSERT_FALSE(hp3.is_invalid());
host_port hp4;
ASSERT_TRUE(hp4.is_invalid());
std::string hp_str2 = "pegasus:8080";
- ASSERT_FALSE(hp4.from_string(hp_str2));
+ hp4 = host_port::from_string(hp_str2);
ASSERT_TRUE(hp4.is_invalid());
host_port hp5("localhost", 8081);
@@ -196,7 +196,7 @@ TEST(host_port_test, rpc_group_host_port)
ASSERT_EQ(2, g_addr->count());
host_port hp_grp2;
- hp_grp2 = host_port(addr_grp);
+ hp_grp2 = host_port::from_address(addr_grp);
ASSERT_EQ(HOST_TYPE_GROUP, hp_grp2.type());
auto g_hp = hp_grp2.group_host_port();
@@ -254,7 +254,7 @@ TEST(host_port_test, dns_resolver)
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());
+ ASSERT_EQ(host_port::from_address(g_addr->leader()), g_hp->leader());
}
}
diff --git a/src/utils/test/json_helper_test.cpp
b/src/utils/test/json_helper_test.cpp
index 978f9e3da..6389e7b0e 100644
--- a/src/utils/test/json_helper_test.cpp
+++ b/src/utils/test/json_helper_test.cpp
@@ -511,7 +511,7 @@ TEST(json_helper, host_port_encode_decode)
dsn::host_port hp("localhost", 8888);
dsn::blob bb = dsn::json::json_forwarder<decltype(hp)>::encode(hp);
dsn::host_port hp2;
- dsn::json::json_forwarder<dsn::host_port>::decode(bb, hp2);
+ ASSERT_TRUE(dsn::json::json_forwarder<dsn::host_port>::decode(bb, hp2));
ASSERT_EQ(hp, hp2);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
