wwbmmm commented on PR #3428: URL: https://github.com/apache/brpc/pull/3428#issuecomment-5568481358
Overall the new UrmaTransport subsystem is well-structured and defensively written (handshake parsing bounds-checks, flow-control credits with overflow validation, robust TCP fallback). One concern in the Bazel build is worth addressing before merge. - `BUILD.bazel:0` The cc_library `srcs` list appends `src/brpc/urma/mock_urma.cpp` unconditionally (in the base source list) and again in the `brpc_with_urma` select. Unlike the Makefile (`URMA_USE_MOCK=0` filters it out) and CMakeLists (`list(REMOVE_ITEM BRPC_SOURCES ... mock_urma.cpp )` when `NOT URMA_USE_MOCK_ENABLED`), the `brpc_with_urma_use_real` select maps to `[]` which does NOT remove the mock translation unit. Since mock_urma.cpp is guarded by `#if BRPC_WITH_URMA`, a real-liburma Bazel build (`--define=BRPC_WITH_URMA=true --define=BRPC_URMA_USE_MOCK=false -lurma`) would still compile and link the `urma_*` mock symbols and clash with the real `liburma`. Please exclude `mock_urma.cpp` under `brpc_with_urma_use_real` (e.g. via a `select` that maps the real case to an empty srcs list), matching the CMake/Make real-library behavior; also note the base list already has it so adding it again in `brpc_with_urma` is redundant. --- 🤖 This reply was automatically generated by brpc-oncall -- 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]
