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! |
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.
|
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 |
|
@wwbmmm Gentle ping. Could you take a look at the proposals for points 2 and 3 above? Once the direction is agreed, I'll implement them in this PR. |
I think that's ok. Please go on. |
|
LGTM |
54fc318 to
e872eae
Compare
3be3c08 to
8fd8b9d
Compare
|
FlatBuffers ON now runs in CI. The remaining failure, FB ON (ubuntu-22.04, cmake, clang), comes from master: in C++14 builds, cacheline-aligned pools are allocated with plain |
Add the optional fb_rpc transport. A frame is a 12-byte FRPC header and fixed request or response metadata, all in network byte order, followed by the FlatBuffers message and an optional attachment. Methods are addressed by stable IDs. Integrate it with Channel, Controller and Server without changing the existing protobuf Protocol callback signatures: - Channel, Controller and Server keep the same layout whether BRPC_WITH_FLATBUFFERS is on or off. Generated stubs reach a brpc::Channel through a library adapter, so `Echo::Stub stub(&channel);` works unchanged. A layout probe built in both configurations checks it. - FlatBuffers methods use Server::MethodProperty, so bvar exposure, concurrency limits, MaxConcurrencyOf() and IgnoreEovercrowdedOf() share one code path. They stay out of the map other protocols search by name. ListFlatBuffersServices() lists them and /status shows them. - A response is accepted only on the socket used by the attempt it answers. 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. Unit tests that registered a private protocol as ID 30, now used by fb_rpc, move to ID 127. The codegen test lists the selected Protobuf include directories before shared prefixes. Add 42 protocol tests covering the wire format, service lifecycle, registry, malformed input and real RPC round trips.
Add a client/server example built from a generated service. By default it runs a bounded loopback smoke test: 13 verified replies, 2 expected schema rejections and a clean shutdown. An opt-in timed mode compares fb_rpc with an equivalent protobuf service over baidu_std on the same port. --duration_s and --warmup_s run closed-loop callers and print QPS, latency percentiles and client CPU as one JSON line; --protocol, --use_bthread and --allow_remote select the protocol, bthread callers and remote endpoints. Request and reply builders reserve the payload size, so large messages are not grown by repeated reallocate-and-copy. The example takes an installed FlatBuffers-enabled bRPC, writes the code generated by flatc and brpc_flatc into its own build directory, and lists the selected Protobuf include directories before shared prefixes.
Add a runner that builds and tests the FlatBuffers-enabled configuration with CMake, Make or Bazel, rejects empty, filtered or skipped test runs, and compares the class layout probes built with FlatBuffers on and off. Add its self-tests and a GitHub Actions workflow that runs it on Linux and macOS when FlatBuffers-related paths change. The jobs build gperftools as ci-linux.yml does. On macOS they use protobuf@29 followed by the shared Homebrew prefix, as ci-macos.yml does.
Add Chinese and English guides covering how to enable FlatBuffers, the dependencies, building the runtime, running the example with its smoke test and timed comparison, the network RPC API, builder sizing for large messages and the FRPC wire format. Update the README files.
8fd8b9d to
a26ffd9
Compare
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: