Copilot commented on code in PR #3010:
URL: https://github.com/apache/brpc/pull/3010#discussion_r2173183494
##########
src/brpc/selective_channel.cpp:
##########
@@ -197,10 +197,14 @@ int ChannelBalancer::AddChannel(ChannelBase* sub_channel,
}
SocketUniquePtr ptr;
int rc = Socket::AddressFailedAsWell(sock_id, &ptr);
- if (rc < 0 || (rc > 0 && !ptr->HCEnabled())) {
+ if (rc < 0) {
LOG(FATAL) << "Fail to address SocketId=" << sock_id;
return -1;
}
+ if (rc > 0 && !ptr->HCEnabled()) {
+ LOG(FATAL) << "Health check of SocketId=" << sock_id << " is disable";
Review Comment:
The error message is grammatically incorrect. Change "is disable" to "is
disabled".
```suggestion
LOG(FATAL) << "Health check of SocketId=" << sock_id << " is
disabled";
```
##########
test/brpc_socket_map_unittest.cpp:
##########
@@ -140,6 +141,32 @@ TEST_F(SocketMapTest, max_pool_size) {
EXPECT_TRUE(ptrs[i]->Failed());
}
}
+
+TEST_F(SocketMapTest, disable_health_check) {
+ int32_t old_interval = brpc::FLAGS_health_check_interval;
+ brpc::FLAGS_health_check_interval = 0;
+ brpc::SocketId id;
+ ASSERT_EQ(-1, brpc::SocketMapFind(g_key, &id));
+ ASSERT_EQ(0, brpc::SocketMapInsert(g_key, &id));
+ ASSERT_EQ(0, brpc::SocketMapFind(g_key, &id));
+ {
+ brpc::SocketUniquePtr ptr;
+ ASSERT_EQ(0, brpc::Socket::Address(id, &ptr));
+ }
+ ASSERT_EQ(0, brpc::Socket::SetFailed(id));
+
+ brpc::SocketUniquePtr ptr;
+ // The socket should not be recycled,
+ // because SocketMap holds a reference to it.
+ ASSERT_EQ(1, brpc::Socket::AddressFailedAsWell(id, &ptr));
+ ASSERT_EQ(2, ptr->nref());
+ brpc::SocketMapRemove(g_key);
+ // After removing the socket, `ptr' holds the last reference.
+ ASSERT_EQ(1, ptr->nref());
+ ASSERT_EQ(-1, brpc::SocketMapFind(g_key, &id));
+ brpc::FLAGS_health_check_interval = old_interval;
Review Comment:
Restoring the flag only at the end of the test can leave side effects if an
assertion fails earlier. Consider using an RAII or scoped helper to reset the
flag even on failures.
```suggestion
```
--
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-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
