Repository navigation
Conversation
0934686 to
48177aa
Compare
|
Hi @chenBright @wwbmmm @zchuango @darion-yaphet, thanks again for all the reviews on #3558! #3559 is the last PR in the FlatBuffers series: the optional Could you please take a look when you have time? Any feedback is welcome. Thanks! |
Add the optional fb_rpc transport with length-delimited metadata and stable method IDs, and integrate it with Channel, Controller and Server without changing the existing protobuf Protocol callback signatures. Keep request buffers immutable across retries and concurrent calls, and manage service ownership, asynchronous completion and concurrency limits. Reject invalid frames and unsupported authentication, compression, checksums and streams. The feature stays disabled by default in CMake, Make and Bazel. Add 33 protocol tests covering the wire format, service lifecycle, malformed input and real RPC round trips.
Add a client/server example built from a generated service, with a bounded loopback smoke test: 13 verified replies, 2 expected schema rejections and a clean shutdown. It is a functional check, not a benchmark. The example takes an installed FlatBuffers-enabled bRPC and writes the code generated by flatc and brpc_flatc into its own build directory.
Add a runner that builds and tests the FlatBuffers-enabled configuration with CMake, Make or Bazel and rejects empty, filtered or skipped test runs. Add its self-tests and a GitHub Actions workflow that runs it on Linux and macOS when FlatBuffers-related paths change.
Add Chinese and English guides covering how to enable FlatBuffers, the dependency and ABI requirements, building the runtime, running the example and its smoke test, the network RPC API and the FRPC wire format. Update the generator README for the RPC example.
48177aa to
8cbb17b
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
_options.protocolis configured as a protocol name (e.g. \"fb_rpc\" / \"baidu_std\"), but here… · New_options.protocolis configured as a protocol name (e.g. \"fb_rpc\" / \"baidu_std\"), but here… · Newstd::unordered_setis used in this file, but the FlatBuffers include block only adds<map>in… · New
What changed in this PR
This PR adds optional FlatBuffers RPC (“fb_rpc”) transport support on top of the existing FlatBuffers message/service generation work, plus an end-to-end example and a dedicated FlatBuffers-ON validation workflow.
Changes:
- Integrates
fb_rpcas a new built-inProtocolwith Channel/Controller/Server support, service registration, and concurrency integration. - Adds comprehensive FlatBuffers protocol/unit tests and wires them into CMake/Make/Bazel test builds.
- Adds a generated client/server example with a bounded smoke test, updated documentation (EN/CN), and a GitHub Actions “FlatBuffers ON” gate runner.
| File | Description |
|---|---|
| tools/flatbuffers/README.md | Updates generator README to point to runtime enablement + fb_rpc usage and verification guidance. |
| test/brpc_flatbuffers_protocol_unittest.cpp | Adds dedicated protocol-level and lifecycle tests for fb_rpc. |
| test/brpc_channel_unittest.cpp | Avoids colliding with newly assigned built-in protocol IDs in channel tests. |
| test/Makefile | Ensures FlatBuffers protocol tests are excluded when FlatBuffers is OFF and builds/link rules include the new test. |
| test/CMakeLists.txt | Adds brpc_flatbuffers_protocol_unittest to CMake test targets when FlatBuffers is ON. |
| test/BUILD.bazel | Adds Bazel cc_test for brpc_flatbuffers_protocol_unittest and excludes it from generic globbed tests. |
| src/brpc/server.h | Introduces FlatBuffers service registration API and internal method property plumbing (guarded by BRPC_WITH_FLATBUFFERS). |
| src/brpc/server.cpp | Implements FlatBuffers service registry, versioning/stat exposure, concurrency limiter hookup, and conflict checks with protobuf services. |
| src/brpc/policy/flatbuffers_protocol.h | Declares the fb_rpc protocol hook points (parse/verify/serialize/pack/process). |
| src/brpc/policy/flatbuffers_protocol.cpp | Implements FRPC framing, request/response processing, and async completion lifecycle for FlatBuffers RPC. |
| src/brpc/options.proto | Reserves built-in PROTOCOL_FLATBUFFERS_RPC = 30. |
| src/brpc/global.cpp | Registers fb_rpc as a built-in protocol when FlatBuffers is enabled. |
| src/brpc/details/server_private_accessor.h | Exposes FlatBuffers method lookup to protocol code via server private accessor. |
| src/brpc/details/controller_private_accessor.h | Adds private setter for FlatBuffers method descriptor on Controller. |
| src/brpc/controller.h | Adds Controller::flatbuffers_method() accessor and stores FlatBuffers method descriptor pointer. |
| src/brpc/controller.cpp | Clears FlatBuffers method context in Controller::ResetPods(). |
| src/brpc/channel.h | Extends Channel to implement flatbuffers::RpcChannel and adds FBCallMethod. |
| src/brpc/channel.cpp | Routes FlatBuffers calls via FBCallMethod, adds entry validation, and adjusts tracing method-name selection. |
| src/brpc/CMakeLists.txt | Ensures flatbuffers_protocol.cpp is excluded when FlatBuffers is OFF. |
| example/benchmark_fb/smoke.py | Adds bounded end-to-end smoke test orchestration for generated FB client/server. |
| example/benchmark_fb/server.cpp | Adds example FlatBuffers server (protocol fb_rpc, service registration, graceful shutdown). |
| example/benchmark_fb/echo.fbs | Defines example schema and stable wire ID for the RPC method. |
| example/benchmark_fb/client.cpp | Adds example FlatBuffers client driving generated stub + verifies response schema/attachments. |
| example/benchmark_fb/README.md | Documents building/running the example and what the smoke test validates. |
| example/benchmark_fb/CMakeLists.txt | Adds standalone CMake build for the example + codegen steps and smoke CTest. |
| docs/en/flatbuffers.md | Expands docs to cover fb_rpc enablement, full verification steps, and limitations. |
| docs/cn/flatbuffers.md | Mirrors EN documentation updates for fb_rpc enablement and verification steps. |
| README_cn.md | Links to the CN FlatBuffers RPC guide. |
| README.md | Links to the EN FlatBuffers RPC guide. |
| Makefile | Excludes flatbuffers_protocol.cpp from Make builds when FlatBuffers is OFF. |
| CMakeLists.txt | Updates WITH_FLATBUFFERS option description to include RPC support. |
| BUILD.bazel | Includes flatbuffers_protocol.cpp in FlatBuffers-enabled Bazel source patterns. |
| .gitignore | Stops ignoring .github/ to keep workflows/scripts discoverable. |
| .github/workflows/flatbuffers-on.yml | Adds a CI workflow that runs the FlatBuffers-ON gates across CMake/Make/Bazel on Linux/macOS. |
| .github/scripts/test_flatbuffers_on.py | Adds self-tests validating the gate script’s report parsing and safety checks. |
| .github/scripts/flatbuffers-on.py | Adds an “ON runner” that builds deps, runs unit/codegen/example tests, and rejects empty/skipped runs. |
Review effort: Lite
| return; | ||
| } | ||
| cntl->set_used_by_rpc(); | ||
| const bool is_fb_protocol = (_options.protocol == PROTOCOL_FLATBUFFERS_RPC); |
| 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) { |
| // 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; |
| 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; | ||
| } |
|
[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
Note: Copilot's three findings above look like false positives to me. Thanks! |
| const uint32_t attachment_size = Load32(meta + 8); | ||
| const bthread_id_t correlation_id = {Load64(meta + 12)}; | ||
| Controller* cntl = nullptr; | ||
| const int rc = bthread_id_lock(correlation_id, reinterpret_cast<void**>(&cntl)); |
There was a problem hiding this comment.
bthread_id_lock() validates a process-wide correlation ID, but does not verify that the response arrived on the socket used by that request. A response carrying another in-flight RPC's valid ID can therefore modify and complete that RPC.
Please follow the existing protobuf response handlers: check that cntl is non-null and call does_response_match_request_socket() before modifying the Controller or response. Unlock and discard the message when validation fails, and add regression coverage for mismatched sockets and stale retry responses.
There was a problem hiding this comment.
Thanks, you're right. ProcessFlatBuffersResponse only checked that the correlation id was still valid, so a response on another connection that carried a live id could complete the wrong RPC.
Fixed in a6709e7 the same way as ProcessRpcResponse: after bthread_id_lock(), if cntl is null or does_response_match_request_socket() fails for the socket the response arrived on, the handler logs a warning, unlocks the id and drops the message before touching the Controller or the response. New regression tests cover a live id arriving on a foreign socket, a response from a previous retry, and a preserved backup attempt that may complete only on its own socket. All three fail without the fix.
The example client and server could only run a bounded loopback smoke test. Add an opt-in timed mode so the same binaries can compare fb_rpc with an equivalent protobuf service over baidu_std: - echo_pb.proto mirrors echo.fbs, and the server serves both protocols on one port. - --duration_s and --warmup_s run closed-loop callers and print QPS, latency percentiles and client CPU as one JSON line. - --protocol selects fb_rpc or baidu_std, --use_bthread runs callers as bthreads, and --allow_remote accepts non-loopback endpoints. - Request and reply builders reserve the payload size, so large messages are not grown by repeated reallocate-and-copy. Defaults are unchanged and the smoke test still passes. The guides now describe the example as a functional check with an optional timed mode.
ProcessFlatBuffersResponse only checked that the correlation id was still live. A response on another connection that carried the id of an in-flight RPC could therefore modify and complete that RPC. Like the protobuf response handlers, check that the Controller exists and that the response arrived on the socket used by the attempt it answers, before touching the Controller or the response. Otherwise log a warning, unlock the id and drop the message. Add regression tests for a live id on a foreign socket, a response from a previous retry, and a preserved backup attempt that may only complete on its own socket. The ON runner now expects 36 protocol tests.
|
Thanks for the detailed review, @wwbmmm, and for checking the Copilot comments. Below are the benchmark results for point 1 and proposals for points 2–4. 1. Motivation and benchmark
To measure it I extended
Summary (fb_rpc vs baidu_std; positive QPS is better, negative CPU is better):
Full results are in the collapsed section at the end. Builder sizing. Not covered: RDMA, attachments, the full matrix with pooled connections, and newer Protobuf releases. Missing features. Authentication, compression, checksums and streaming are out of scope for this PR. Because the metadata carries an explicit length, fields such as log-id and user fields can be appended later without breaking the known prefix. 2. ABIAgreed that the layout should not depend on
With this, a release has one layout whatever the option is, and protobuf-only users are not affected. A consumer built with ON against a library built with OFF fails at link time instead of at run time. A separate component would still need protocol registration and dispatch hooks in 3. Service registryProposal:
A shared registry abstraction for both kinds of service would touch the protobuf paths much more, so I'd prefer to do it in a follow-up PR. 4. Wire formatAgreed. Compatibility with the earlier experimental FRPC formats is not a goal, so the header and the fixed metadata will both be big-endian (network order), using the butil byte-order helpers, with a test that fixes the byte layout of request and response frames. The FlatBuffers payload stays little-endian as FlatBuffers defines it; it is not part of the frame. I'll keep the The benchmark changes to Full results (1 / 16 / 64 callers, 10 payload sizes, medians of 5 rounds)1 caller
16 callers
64 callers
For each case the two protocols ran back to back in the same round on the same server process. 15 of the 30 cases were measured in a second session (0 B, 256 B, 4 KB and 16 KB at all caller counts, and every size at 16 callers); results from the two sessions differ by a few percent on their own, so compare cases within a table row rather than across sessions. Reproduce with the example (server and client hosts separate): benchmark_fb_server --listen_addr=SERVER_IP:8901 --allow_remote --duration_s=3600 --max_concurrency=4096
for p in fb_rpc baidu_std; do
benchmark_fb_client --server=SERVER_IP:8901 --allow_remote --protocol=$p --use_bthread=true \
--duration_s=30 --warmup_s=3 --thread_num=64 --request_size=1024
done |
What problem does this PR solve?
This continues the RPC work in #3197 on top of #3558, which is now merged. #3558 added the FlatBuffers messages, builders, service descriptors and the
brpc_flatcgenerator; this PR adds the optionalfb_rpctransport on top of it.The branch is rebased onto master after #3558 and contains only the four RPC commits.
What is changed and the side effects?
Add FlatBuffers RPC protocol support: optionalfb_rpctransport, Channel/Controller/Server integration, service lifecycle handling, attachments, retries and concurrency, with 33 protocol tests.Add a generated FlatBuffers RPC example: generated client/server example with a bounded loopback smoke test. This is functional validation, not a performance benchmark.Add explicit FlatBuffers ON validation: an ON validation runner, its self-tests and a GitHub Actions workflow.Document FlatBuffers setup and example checks: Chinese and English setup, dependency, ABI and example-verification documentation.The transport uses explicit metadata lengths, so optional fields can be appended later while the known prefix stays readable. Existing protobuf-facing
Protocolcallback signatures are unchanged.Side effects and limits:
output/include. The ON runner and the docs now install the runtime into a prefix before building the example.Validation
Run on the rebased head
48177aa2:LINK_SO=ON)python3 .github/scripts/test_flatbuffers_on.pyThe example smoke verifies 13 replies, 2 expected schema rejections and a clean shutdown.
Not covered by these runs: Make and Bazel ON modes (left to CI), sanitizer runs and performance benchmarks.
Check List: