wwbmmm commented on PR #3559: URL: https://github.com/apache/brpc/pull/3559#issuecomment-6396253924
[replied by brpc-oncall robot] Thanks for splitting the RPC increment out of #3197 and for the very thorough documentation. Because this PR adds a new built-in protocol together with public API on `Channel`/`Server` (and changes their layout when `BRPC_WITH_FLATBUFFERS` is on), I'd like to settle a few architecture points before going through the implementation line by line. 1. **Motivation and cost/benefit.** The PR explicitly claims no performance improvement yet, and the example is functional validation only. A new wire protocol is a permanent maintenance commitment (framing, compatibility, tests, docs), and `fb_rpc` gives up authentication, compression, checksums, streaming, log-id/user-field propagation and RPC dump that protocols such as `baidu_std` already provide. Could you add a short design note (or point to the agreed design in #3197) that states the concrete use cases this transport targets and why they cannot be served today, ideally with at least a micro-benchmark of `fb_rpc` against a protobuf/`baidu_std` baseline? Without that it is hard to judge whether the benefit justifies the cost. 2. **ABI.** With `BRPC_WITH_FLATBUFFERS=1`, `Channel` gains a second base class and `Channel`/`Controller`/`Server` change layout, so the library and every consumer must be rebuilt with the same setting. The docs are honest about it, but this effectively rules out packaged/prebuilt brpc and doubles the ABI configurations the project has to reason about. Is that acceptable long term, or could the transport live in a separate optional component/target (or avoid changing core class layouts)? 3. **Parallel service registry.** `Server` now keeps a second registry that re-implements ownership, duplicate/conflict checks, the version string, bvar exposure, concurrency-limiter setup and string-based per-method option lookup for FlatBuffers methods, while `ListServices` and the status surfaces still only see protobuf services (although `GetStat().user_service_count` now includes FlatBuffers services). Every future per-method option must be added twice or will silently not apply to FlatBuffers services. Would it be feasible to factor this into a shared internal registry abstraction? At minimum the `user_service_count` vs. `ListServices` divergence should be resolved, and keeping a full-name index in the registry (as protobuf does with `_method_map`) would avoid the O(services x methods) name lookups. 4. **Wire format.** Since compatibility with the old experimental FRPC variants is explicitly not promised, retaining their mixed endianness (big-endian header, little-endian metadata) and the `FRPC` magic adds complexity without a stated benefit. Before the format is frozen it would be cheaper to use one endianness throughout; if the legacy layout must be preserved, please state why in the docs, as readers will otherwise assume it is accidental. Note: Copilot's three findings above look like false positives to me. `AdaptiveProtocolType` has `operator ProtocolType()`, so `_options.protocol == PROTOCOL_FLATBUFFERS_RPC` compiles and is the idiom already used elsewhere in `channel.cpp`; `server.cpp` already includes `<unordered_set>` at the top; and the `is_fb == is_fb_protocol` guard deliberately avoids calling a protocol's protobuf `get_method_name` hook with a null method descriptor (`GetFlatBuffersMethodName` is installed for `fb_rpc`). Thanks! -- 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]
