You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2022/11/29 08:20:31 UTC
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1251: refactor(rpc_address): simple refactor on rpc_address
empiredan commented on code in PR #1251:
URL: https://github.com/apache/incubator-pegasus/pull/1251#discussion_r1034371967
##########
src/block_service/fds/fds_service.cpp:
##########
@@ -15,31 +15,33 @@
// specific language governing permissions and limitations
// under the License.
-#include "fds_service.h"
+#include "block_service/fds/fds_service.h"
-#include <galaxy_fds_client.h>
+#include <string.h>
+
Review Comment:
```suggestion
```
##########
src/block_service/fds/fds_service.cpp:
##########
@@ -15,31 +15,33 @@
// specific language governing permissions and limitations
// under the License.
-#include "fds_service.h"
+#include "block_service/fds/fds_service.h"
-#include <galaxy_fds_client.h>
+#include <string.h>
Review Comment:
```suggestion
#include <cstring>
```
##########
src/runtime/rpc/group_address.h:
##########
@@ -24,42 +24,37 @@
* THE SOFTWARE.
*/
-/*
- * Description:
- * group_address is a collection of rpc_addresses, usually used for replication
- *
- * Revision history:
- * Sep., 2015, @imzhenyu, first version
- * xxxx-xx-xx, author, fix bug about xxx
- */
-
#pragma once
#include <algorithm>
-#include "utils/api_utilities.h"
+
#include "runtime/api_layer1.h"
-#include "utils/synchronize.h"
+#include "runtime/rpc/rpc_address.h"
+#include "utils/api_utilities.h"
#include "utils/autoref_ptr.h"
#include "utils/rand.h"
-#include "utils/rpc_address.h"
+#include "utils/synchronize.h"
namespace dsn {
-class rpc_group_address : public dsn::ref_counter
+class rpc_group_address : public ref_counter
{
public:
rpc_group_address(const char *name);
rpc_group_address(const rpc_group_address &other);
rpc_group_address &operator=(const rpc_group_address &other);
- bool add(rpc_address addr);
- void add_list(const std::vector<rpc_address> &list)
+ bool add(rpc_address addr); // TODO(yingchun): WARN_UNUSED_RESULT
+ void add_list(const std::vector<rpc_address> &addrs)
{
- for (const rpc_address &r : list) {
- add(r);
+ for (const rpc_address &addr : addrs) {
Review Comment:
```suggestion
for (const auto &addr : addrs) {
```
##########
src/runtime/rpc/group_address.h:
##########
@@ -175,8 +177,9 @@ inline bool rpc_group_address::remove(rpc_address addr)
auto it = std::find(_members.begin(), _members.end(), addr);
bool r = (it != _members.end());
Review Comment:
Refactor as this ?
```c++
if (it == _members.end()) {
return false;
}
if (-1 != _leader_index && addr == _members[_leader_index]) {
_leader_index = -1;
}
_members.erase(it);
return true;
```
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -51,15 +54,15 @@ uint32_t rpc_address::ipv4_from_host(const char *name)
addr.sin_family = AF_INET;
if ((addr.sin_addr.s_addr = inet_addr(name)) == (unsigned int)(-1)) {
+ // TODO(yingchun): use getaddrinfo instead
hostent *hp = ::gethostbyname(name);
- int err = h_errno;
-
- if (hp == nullptr) {
- LOG_ERROR("gethostbyname failed, name = %s, err = %d.", name, err);
+ if (dsn_unlikely(hp == nullptr)) {
+ LOG_ERROR_F(
+ "gethostbyname failed, name = {}, err = {}", name, utils::safe_strerror(h_errno));
Review Comment:
Human-readable message for `::gethostbyname` can be got by `hstrerror(h_errno)`, however considering that soon we will use `getaddrinfo`, we can use `gai_strerror` instead then.
##########
src/runtime/rpc/group_address.h:
##########
@@ -24,42 +24,37 @@
* THE SOFTWARE.
*/
-/*
- * Description:
- * group_address is a collection of rpc_addresses, usually used for replication
- *
- * Revision history:
- * Sep., 2015, @imzhenyu, first version
- * xxxx-xx-xx, author, fix bug about xxx
- */
-
#pragma once
#include <algorithm>
-#include "utils/api_utilities.h"
+
#include "runtime/api_layer1.h"
-#include "utils/synchronize.h"
+#include "runtime/rpc/rpc_address.h"
+#include "utils/api_utilities.h"
#include "utils/autoref_ptr.h"
#include "utils/rand.h"
-#include "utils/rpc_address.h"
+#include "utils/synchronize.h"
namespace dsn {
-class rpc_group_address : public dsn::ref_counter
+class rpc_group_address : public ref_counter
{
public:
rpc_group_address(const char *name);
rpc_group_address(const rpc_group_address &other);
rpc_group_address &operator=(const rpc_group_address &other);
- bool add(rpc_address addr);
- void add_list(const std::vector<rpc_address> &list)
+ bool add(rpc_address addr); // TODO(yingchun): WARN_UNUSED_RESULT
+ void add_list(const std::vector<rpc_address> &addrs)
{
- for (const rpc_address &r : list) {
- add(r);
+ for (const rpc_address &addr : addrs) {
+ // TODO(yingchun): add LOG_WARNING_IF/LOG_ERROR_IF
+ if (!add(addr)) {
+ LOG_WARNING_F("duplicate adress {}", addr);
+ }
}
}
void set_leader(rpc_address addr);
- bool remove(rpc_address addr);
- bool contains(rpc_address addr);
+ bool remove(rpc_address addr); // TODO(yingchun): WARN_UNUSED_RESULT
+ bool contains(rpc_address addr); // TODO(yingchun): WARN_UNUSED_RESULT
Review Comment:
```suggestion
bool contains(rpc_address addr) const; // TODO(yingchun): WARN_UNUSED_RESULT
```
##########
src/runtime/rpc/rpc_address.h:
##########
@@ -151,10 +134,15 @@ class rpc_address
// and you MUST ensure that _addr is INITIALIZED before you call this function
void set_invalid();
- bool operator==(::dsn::rpc_address r) const
+ bool operator==(rpc_address r) const
Review Comment:
```suggestion
bool operator==(const rpc_address &r) const
```
##########
src/runtime/rpc/rpc_address.h:
##########
@@ -166,9 +154,9 @@ class rpc_address
}
}
- bool operator!=(::dsn::rpc_address r) const { return !(*this == r); }
+ bool operator!=(rpc_address r) const { return !(*this == r); }
Review Comment:
```suggestion
bool operator!=(const rpc_address &r) const { return !(*this == r); }
```
##########
src/runtime/rpc/rpc_address.h:
##########
@@ -166,9 +154,9 @@ class rpc_address
}
}
- bool operator!=(::dsn::rpc_address r) const { return !(*this == r); }
+ bool operator!=(rpc_address r) const { return !(*this == r); }
- bool operator<(::dsn::rpc_address r) const
+ bool operator<(rpc_address r) const
Review Comment:
```suggestion
bool operator<(const rpc_address &r) const
```
##########
src/utils/utils.h:
##########
@@ -28,8 +28,10 @@
#include <functional>
#include <memory>
+#include <map>
Review Comment:
```suggestion
#include <map>
#include <memory>
```
--
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