wwbmmm commented on code in PR #3535:
URL: https://github.com/apache/brpc/pull/3535#discussion_r4026322460
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -1480,16 +915,16 @@ void RdmaEndpoint::DeallocateResources() {
DeallocateCq(_resource->send_cq);
DeallocateCq(_resource->recv_cq);
- if (_resource->comp_channel != nullptr) {
- if (_cq_sid != INVALID_SOCKET_ID) {
+ if (nullptr != _resource->comp_channel) {
// Destroy send_comp_channel will destroy this fd,
// so that we should remove it from epoll fd first
int fd = _resource->comp_channel->fd;
- GetGlobalEventDispatcher(fd,
_socket->_io_event.bthread_tag()).RemoveConsumer(fd);
+ GetGlobalEventDispatcher(fd, _socket->_io_event.bthread_tag())
+ .RemoveConsumer(fd);
Review Comment:
[replied by brpc-oncall robot] The inner `if (_cq_sid != INVALID_SOCKET_ID)`
guard was dropped during the reformatting. `RemoveConsumer(comp_channel->fd)`
is now called even when this endpoint never registered that fd with the event
dispatcher, i.e. when `DeallocateResources()` runs after `AllocateResources()`
but before `StartCqEvents()` (which is exactly what happens on TCP fallback via
`DeactivateUpgrade()` -> `RdmaEndpoint::Reset()`). Calling
`epoll_ctl(EPOLL_CTL_DEL)` on an fd that was never added fails with ENOENT and
logs misleading "Fail to remove fd" warnings. Please restore the `_cq_sid`
guard (and keep `remove_consumer` as before).
##########
src/brpc/rdma_transport.cpp:
##########
@@ -70,31 +68,54 @@ int RdmaTransport::Reset(int32_t expected_nref) {
}
std::shared_ptr<AppConnect> RdmaTransport::Connect() {
- if (_default_connect == nullptr) {
- return std::make_shared<rdma::RdmaConnect>();
- }
- return _default_connect;
+ return _default_connect;
+}
+
+void RdmaTransport::SetHighSpeedAvailable(bool available) {
Review Comment:
[replied by brpc-oncall robot] This new code uses 2-space indentation while
the rest of the file/repo follows the 4-space style, and the PR reindents many
unrelated blocks (rdma_transport.cpp, rdma_endpoint.cpp, ub_endpoint.cpp).
Please keep 4-space indentation and separate pure reformatting from functional
changes, so the actual logic diffs stay reviewable and accidental drops such as
the `_cq_sid` guard above are not buried in whitespace.
--
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]
