You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "GehaFearless (via GitHub)" <gi...@apache.org> on 2023/12/21 15:10:04 UTC
[PR] feat(fqdn): json decode & encode for rpc_host_port [incubator-pegasus]
GehaFearless opened a new pull request, #1811:
URL: https://github.com/apache/incubator-pegasus/pull/1811
Issue: https://github.com/apache/incubator-pegasus/issues/1810
Implement json_decode & json_encode for class `rpc_host_port`.
--
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
Re: [PR] feat(fqdn): json decode & encode for rpc_host_port [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1811:
URL: https://github.com/apache/incubator-pegasus/pull/1811#discussion_r1436458989
##########
src/utils/string_conv.h:
##########
@@ -124,6 +124,10 @@ inline bool buf2uint64(absl::string_view buf, uint64_t &result)
return internal::buf2unsigned(buf, result);
}
+inline bool buf2uint16(absl::string_view buf, uint16_t &result)
+{
+ return internal::buf2unsigned(buf, result);
+}
Review Comment:
```suggestion
}
```
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,32 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string &s)
+{
+ const auto pos = s.find_last_of(':');
+ if (pos == std::string::npos) {
Review Comment:
```suggestion
if (dsn_unlikely(pos == std::string::npos)) {
```
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,32 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string &s)
+{
+ const auto pos = s.find_last_of(':');
+ if (pos == std::string::npos) {
+ return false;
+ }
+ _host = s.substr(0, pos);
+ std::string port = s.substr(pos + 1);
+
+ if (!dsn::buf2uint16(port, _port)) {
Review Comment:
```suggestion
if (dsn_unlikely(!dsn::buf2uint16(port, _port))) {
```
--
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
Re: [PR] feat(fqdn): json decode & encode for rpc_host_port [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1811:
URL: https://github.com/apache/incubator-pegasus/pull/1811#discussion_r1436421062
##########
src/runtime/test/host_port_test.cpp:
##########
@@ -87,6 +87,16 @@ TEST(host_port_test, operators)
ASSERT_NE(hp, hp2);
ASSERT_FALSE(hp.is_invalid());
ASSERT_TRUE(hp2.is_invalid());
+
+ std::string hp_str = "localhost:8080";
+ host_port hp3;
+ ASSERT_TRUE(hp3.from_string(hp_str));
Review Comment:
```suggestion
ASSERT_TRUE(hp3.is_invalid());
ASSERT_TRUE(hp3.from_string(hp_str));
```
##########
src/runtime/test/host_port_test.cpp:
##########
@@ -87,6 +87,16 @@ TEST(host_port_test, operators)
ASSERT_NE(hp, hp2);
ASSERT_FALSE(hp.is_invalid());
ASSERT_TRUE(hp2.is_invalid());
+
+ std::string hp_str = "localhost:8080";
+ host_port hp3;
+ ASSERT_TRUE(hp3.from_string(hp_str));
+ ASSERT_EQ(hp, hp3);
+
+ host_port hp4;
+ std::string hp_str2 = "pegasus:8080";
+ ASSERT_FALSE(hp4.from_string(hp_str2));
Review Comment:
```suggestion
ASSERT_TRUE(hp4.is_invalid());
ASSERT_FALSE(hp4.from_string(hp_str2));
```
##########
src/runtime/test/host_port_test.cpp:
##########
@@ -87,6 +87,16 @@ TEST(host_port_test, operators)
ASSERT_NE(hp, hp2);
ASSERT_FALSE(hp.is_invalid());
ASSERT_TRUE(hp2.is_invalid());
+
+ std::string hp_str = "localhost:8080";
+ host_port hp3;
+ ASSERT_TRUE(hp3.from_string(hp_str));
+ ASSERT_EQ(hp, hp3);
Review Comment:
```suggestion
ASSERT_EQ(hp, hp3);
ASSERT_FALSE(hp3.is_invalid());
```
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,33 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string s)
Review Comment:
```suggestion
bool host_port::from_string(const std::string &s)
```
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,33 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string s)
+{
+ auto pos = s.find_last_of(':');
Review Comment:
```suggestion
const auto pos = s.find_last_of(':');
```
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,33 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string s)
+{
+ auto pos = s.find_last_of(':');
+ if (pos == std::string::npos) {
+ return false;
+ }
+ _host = s.substr(0, pos);
+ std::string port = s.substr(pos + 1);
+
+ if (!internal::buf2unsigned(port, _port) || _port > UINT16_MAX) {
Review Comment:
Consider add a new function `buf2uint16` ? In principle, `internal::buf2unsigned` is an internal function and should be used only in `src/utils/string_conv.h`.
On the other hand, there is no need to check the validity of `_port`, which has been done in `buf2unsigned`:
```suggestion
if (!buf2uint16(port, _port)) {
```
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,33 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string s)
+{
+ auto pos = s.find_last_of(':');
+ if (pos == std::string::npos) {
+ return false;
+ }
+ _host = s.substr(0, pos);
+ std::string port = s.substr(pos + 1);
+
+ if (!internal::buf2unsigned(port, _port) || _port > UINT16_MAX) {
+ return false;
+ }
+
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ AddrInfo result;
+ auto err_code = GetAddrInfo(_host, hints, &result);
+ if (dsn_unlikely(!err_code.is_ok())) {
Review Comment:
```suggestion
if (dsn_unlikely(!GetAddrInfo(_host, hints, &result))) {
```
--
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
Re: [PR] feat(fqdn): json decode & encode for rpc_host_port [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1811:
URL: https://github.com/apache/incubator-pegasus/pull/1811#discussion_r1436417805
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,33 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string s)
+{
+ auto pos = s.find_last_of(':');
+ if (pos == std::string::npos) {
+ return false;
+ }
+ _host = s.substr(0, pos);
+ std::string port = s.substr(pos + 1);
+
+ if (!internal::buf2unsigned(port, _port) || _port > UINT16_MAX) {
Review Comment:
Consider adding a new function `buf2uint16` ? In principle, `internal::buf2unsigned` is an internal function and should be used only in `src/utils/string_conv.h`.
On the other hand, there is no need to check the validity of `_port`, which has been done in `buf2unsigned`:
```suggestion
if (!buf2uint16(port, _port)) {
```
--
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
Re: [PR] feat(fqdn): json decode & encode for rpc_host_port [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 merged PR #1811:
URL: https://github.com/apache/incubator-pegasus/pull/1811
--
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
Re: [PR] feat(fqdn): json decode & encode for rpc_host_port [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1811:
URL: https://github.com/apache/incubator-pegasus/pull/1811#discussion_r1436890212
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,32 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string &s)
+{
+ const auto pos = s.find_last_of(':');
+ if (dsn_unlikely(pos == std::string::npos)) {
+ return false;
+ }
+ _host = s.substr(0, pos);
+ std::string port = s.substr(pos + 1);
+
+ if (dsn_unlikely(!dsn::buf2uint16(port, _port))) {
+ return false;
+ }
+
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ AddrInfo result;
+ if (dsn_unlikely(!GetAddrInfo(_host, hints, &result))) {
Review Comment:
```suggestion
if (dsn_unlikely(!GetAddrInfo(_host, hints, nullptr))) {
```
--
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
Re: [PR] feat(fqdn): json decode & encode for rpc_host_port [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1811:
URL: https://github.com/apache/incubator-pegasus/pull/1811#discussion_r1436280678
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,34 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string s)
+{
+ auto pos = s.find_last_of(':');
+ if (pos == std::string::npos) {
+ return false;
+ }
+ _host = s.substr(0, pos);
+ std::string port = s.substr(pos + 1);
+
+ if (!internal::buf2unsigned(port, _port) || _port > UINT16_MAX) {
+ return false;
+ }
+
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ AddrInfo result;
+ auto err_code = GetAddrInfo(_host, hints, &result);
+
Review Comment:
```suggestion
```
##########
src/utils/test/json_helper_test.cpp:
##########
@@ -505,4 +506,14 @@ TEST(json_helper, upgrade_downgrade)
ASSERT_EQ(n2.c, o.c);
}
+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<decltype(hp)>::decode(bb, hp2);
Review Comment:
```suggestion
dsn::json::json_forwarder<dsn::host_port>::decode(bb, hp2);
```
--
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