Copilot commented on code in PR #3559:
URL: https://github.com/apache/brpc/pull/3559#discussion_r4357817022
##########
src/brpc/channel.cpp:
##########
@@ -538,12 +564,17 @@ void Channel::CallMethod(const
google::protobuf::MethodDescriptor* method,
return;
}
cntl->set_used_by_rpc();
+ const bool is_fb_protocol = (_options.protocol ==
PROTOCOL_FLATBUFFERS_RPC);
Review Comment:
`_options.protocol` is configured as a protocol name (e.g. \"fb_rpc\" /
\"baidu_std\"), but here it’s compared to the numeric enum
`PROTOCOL_FLATBUFFERS_RPC`. This is either a compile error or will behave
incorrectly depending on the actual field type. Compare against the protocol
name (\"fb_rpc\") or against the channel’s resolved `ProtocolType` (whichever
the codebase uses consistently) so that `FBCallMethod` gating and tracing
method-name logic work correctly.
##########
src/brpc/channel.cpp:
##########
@@ -538,12 +564,17 @@ void Channel::CallMethod(const
google::protobuf::MethodDescriptor* method,
return;
}
cntl->set_used_by_rpc();
+ const bool is_fb_protocol = (_options.protocol ==
PROTOCOL_FLATBUFFERS_RPC);
if (cntl->_sender == nullptr && IsTraceable(Span::tls_parent().get())) {
const int64_t start_send_us = butil::cpuwide_time_us();
std::string method_name;
- if (_get_method_name) {
+ if (_get_method_name && is_fb == is_fb_protocol) {
Review Comment:
`_options.protocol` is configured as a protocol name (e.g. \"fb_rpc\" /
\"baidu_std\"), but here it’s compared to the numeric enum
`PROTOCOL_FLATBUFFERS_RPC`. This is either a compile error or will behave
incorrectly depending on the actual field type. Compare against the protocol
name (\"fb_rpc\") or against the channel’s resolved `ProtocolType` (whichever
the codebase uses consistently) so that `FBCallMethod` gating and tracing
method-name logic work correctly.
##########
src/brpc/server.cpp:
##########
@@ -1656,6 +1762,125 @@ int Server::AddService(google::protobuf::Service*
service,
return AddServiceInternal(service, false, options);
}
+#if BRPC_WITH_FLATBUFFERS
+int Server::AddFlatBuffersService(flatbuffers::Service* service,
+ ServiceOwnership ownership) {
+ if (service == nullptr ||
+ (ownership != SERVER_OWNS_SERVICE &&
+ ownership != SERVER_DOESNT_OWN_SERVICE)) {
+ LOG(ERROR) << "Invalid FlatBuffers service or ownership";
+ return -1;
+ }
+ if (InitializeOnce() != 0 || status() != READY) {
+ LOG(ERROR) << "Can't add FlatBuffers service to Server[" << version()
+ << "] which is " << status_str(status());
+ return -1;
+ }
+ const flatbuffers::ServiceDescriptor* sd = service->GetDescriptor();
+ if (sd == nullptr || sd->name().empty() || sd->full_name().empty() ||
+ sd->method_count() <= 0) {
+ LOG(ERROR) << "Invalid FlatBuffers service descriptor";
+ return -1;
+ }
+ if (_flatbuffers_services) {
+ for (const auto& entry : _flatbuffers_services->services) {
+ if (entry.second->service == service || entry.first == sd->index()
||
+ entry.second->descriptor->full_name() == sd->full_name()) {
+ LOG(ERROR) << "Duplicate FlatBuffers service or hash
collision: "
+ << sd->full_name();
+ return -1;
+ }
+ }
+ }
+
+ // Keep ownership with the caller until the entire record is published.
+ auto record = std::make_unique<FlatBuffersServiceMap::Service>(service,
sd);
+ std::unordered_set<std::string> method_names;
Review Comment:
`std::unordered_set` is used in this file, but the FlatBuffers include block
only adds `<map>` in the diff. This will fail to compile unless another header
incidentally provides it. Add an explicit `#include <unordered_set>` under `#if
BRPC_WITH_FLATBUFFERS` (near the existing `<map>` include).
##########
src/brpc/server.cpp:
##########
@@ -114,6 +118,58 @@ const char* status_str(Server::Status s) {
return "UNKNOWN_STATUS";
}
+#if BRPC_WITH_FLATBUFFERS
+struct Server::FlatBuffersServiceMap {
+ struct Method {
+ Method(flatbuffers::Service* service,
+ const flatbuffers::MethodDescriptor* descriptor)
+ : status(std::make_unique<MethodStatus>())
+ , property{service, descriptor, status.get(), false} {}
+
+ std::unique_ptr<MethodStatus> status;
+ FlatBuffersMethodProperty property;
+ AdaptiveMaxConcurrency max_concurrency;
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(Method);
+ };
+ struct Service {
+ Service(flatbuffers::Service* svc,
+ const flatbuffers::ServiceDescriptor* desc)
+ : service(svc)
+ , descriptor(desc)
+ , ownership(SERVER_DOESNT_OWN_SERVICE) {}
+ ~Service() {
+ methods.clear();
+ if (ownership == SERVER_OWNS_SERVICE) {
+ delete service;
+ }
+ }
+
+ flatbuffers::Service* service;
+ const flatbuffers::ServiceDescriptor* descriptor;
+ ServiceOwnership ownership;
+ std::map<int32_t, std::unique_ptr<Method> > methods;
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(Service);
+ };
+ typedef std::map<uint32_t, std::unique_ptr<Service> > ServiceMap;
+ ServiceMap services;
+
+ Method* FindMethodByFullName(const butil::StringPiece& fullname) const {
+ for (const auto& service : services) {
+ for (const auto& method : service.second->methods) {
+ if (method.second->property.method->full_name() == fullname) {
+ return method.second.get();
+ }
+ }
+ }
+ return nullptr;
+ }
Review Comment:
FlatBuffers method lookups by full name are O(#services × #methods) due to
nested iteration. This appears in configuration/management paths (e.g.,
conflict checks and string-based `MaxConcurrencyOf` / `IgnoreEovercrowdedOf`).
Consider maintaining an additional `unordered_map<std::string, Method*>` index
(built at service registration time and updated on removal/clear) so name-based
lookups are O(1) and don’t scale poorly as the number of registered services
grows.
--
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]
