sync: address review comments + merge apache/master (fixes ASAN CI) for #3428 - #7
Closed
cw20050111-prog wants to merge 48 commits into
Closed
sync: address review comments + merge apache/master (fixes ASAN CI) for #3428#7cw20050111-prog wants to merge 48 commits into
cw20050111-prog wants to merge 48 commits into
Conversation
PollCq only re-polled send_cq after arming both CQs, so a recv CQE arriving in the one-shot notification race window of recv_cq was left in the CQ and the RPC timed out. Restart the re-poll from recv_cq so that both CQs are covered.
Keep the current RPC span alive from Controller until the RPC finishes, SubmitSpan runs, or the Controller is reset. This lets root client spans without a local parent be submitted to rpcz instead of being destroyed after the caller-side temporary shared_ptr goes out of scope. Child client spans remain linked to their parent through weak local-parent references and parent-owned client lists, so they are still serialized under their parent without introducing shared_ptr cycles. Co-authored-by: lh2debug <lh2debug@163.com>
Support Protobuf v35
Wait for the asynchronous span collector to release root client spans after Controller reset or submission. This prevents LeakSanitizer from reporting pending test spans when the short-lived test binary exits before the collector’s first polling interval. Signed-off-by: Zhengwei Zhu <141622927+ZhengweiZhu@users.noreply.github.com>
Add the missing BRPC_WITH_UBRING Bazel configuration, wire it through the library and example targets, and document the supported build commands. Enable RDMA and UBRING in CI jobs whose all-options configurations did not exercise those features. Run both feature suites in the existing Bazel unit-test jobs and enable RDMA in the Make unit-test job. Use unsigned literals for UBRING atomic counter operations to match the counter type and avoid template deduction failures on stricter compilers. Signed-off-by: Zhengwei Zhu <141622927+ZhengweiZhu@users.noreply.github.com>
…ization and butil/time (apache#3439)
) * cap simple string length in RedisReply::ConsumePartialIOBuf Signed-off-by: ubeddulla khan <ubed@bugqore.com> * reject negative redis_max_allocation_size in simple string branch Signed-off-by: ubeddulla khan <ubed@bugqore.com> * enforce redis simple string cap while waiting for CRLF Signed-off-by: ubeddulla khan <ubed@bugqore.com> --------- Signed-off-by: ubeddulla khan <ubed@bugqore.com>
* add the progressive timeout reader * optimize the code format * WIP: progressive read timeout review * test: strengthen progressive read timeout coverage
) * Limit mcpack2pb array item count to the actual payload size The item count in an mcpack array header is read directly from the request and was used as-is by the generated parsing code to Reserve() memory for repeated protobuf fields. A malformed request could claim an item count up to INT32_MAX and force the server to preallocate ~16GB of virtual memory, which may abort the process on memory-constrained hosts. Cap the item count by the remaining bytes of the array (each item occupies at least one byte) so that the preallocation is bounded by the request size. * Fix underflow in mcpack2pb array item count clamping
…#3452) * Fix RTMP abort message deleting the chunk stream being parsed * Address review comments on RTMP abort regression test
ParseSofaMessage only checked body_size against max_body_size, while meta_size and the total frame size were left unbounded. A frame with a large meta_size and zero body_size passed the body_size check and made the connection keep buffering far beyond the configured limit before the invalid metadata was rejected. Bound meta_size by max_body_size as well, consistent with other protocols such as baidu_std and hulu_pbrpc. Add unit tests covering oversized body and oversized metadata.
* Support flow-controlled gRPC client requests - Split client request DATA frames according to the peer's connection and stream flow-control windows. - Buffer unsent DATA and resume transmission when WINDOW_UPDATE restores capacity. - Track pending request bytes per H2 connection and apply socket_max_unwritten_bytes as an upper bound. - Reject or reroute new requests once the pending DATA limit is reached. - Release buffered DATA when an RPC fails, times out, or its stream is removed. - Add tests for fragmented transmission, deferred DATA flushing, pending-byte accounting, and buffer cleanup. * Fix pending HTTP/2 data limit race - Check pending DATA capacity atomically with client stream insertion. - Leave stream and window state unchanged when the limit is exceeded. - Use an ephemeral port in the gRPC flow-control test. - Avoid accessing an empty payload buffer in H2 frame tests.
This reverts commit e0abb10.
) * fix WeightedRandomizedLoadBalancer skipping the last server SelectServer() draws random_weight from fast_rand_less_than(weight_sum), i.e. from [0, weight_sum - 1], and then lower_bound()s it against Server::current_weight_sum, which Add() fills with an inclusive prefix sum. lower_bound() returns the first server whose prefix sum is >= random_weight, but a server owns the half-open range [prefix(i-1), prefix(i)), so the predicate has to be > random_weight. Because of that the first server in the list also serves random_weight == prefix(0) and the last server never serves anything at all, since random_weight can never reach weight_sum. With four servers of equal weight the measured distribution is 49.8/25.1/25.1/0.0 percent instead of 25 percent each. Search for random_weight + 1 so that lower_bound() lands on the first prefix sum strictly greater than random_weight. The existing weighted_randomized test does not catch this: its servers have weights 3/2/5/10 and it only asserts that each rate is within 0.5x~2x of the expected one. The weight-10 server measures 0.448 before this change and 0.494 after it, both inside that band. Add weighted_randomized_equal_weight, which uses equal weights so that a single misplaced slot is visible, and check the rates within 0.9x~1.1x. Signed-off-by: Anas <156536069+Nas01010101@users.noreply.github.com> * use upper_bound for the weighted prefix-sum search upper_bound(random_weight) states the intent directly: the first server whose inclusive prefix sum is strictly greater than random_weight. It is the same search as lower_bound(random_weight + 1) without the increment. Also correct the tolerance comment in the unit test: with run_times=40000 and p=0.25 the count has sigma ~= 86.6, so the 0.9x~1.1x band is about 11 sigma, not more than 20. --------- Signed-off-by: Anas <156536069+Nas01010101@users.noreply.github.com>
* Fix bug when parsing zero-length string field in mcpack2pb UnparsedValue::as_string() resizes the output string to without checking , where is the value_size of a string field read from the input. When value_size is 0, underflows to SIZE_MAX and resize() throws std::length_error, which is not caught on the request path and therefore crashes the server. Reject such malformed fields by marking the stream bad so the caller can fail the request gracefully instead. * Clear output string when size error
* Refactor NULL with nullptr in brpc/policy * Fix NULL in mysql comments
- Rename ack_bit_is_rdma_ok to ack_bit_is_urma_ok. - Replace hard-coded IOBuf block header size with sizeof(butil::IOBuf::Block). - Pin UMDK dependency to a specific commit instead of a mutable tag.
Reviewer noted docs/cn/urma.md only covered the CMake build and that DOWNLOAD_URMA_HEADERS has no Bazel equivalent. Document the Bazel build command and clarify that Bazel always fetches UMDK headers from the pinned git_repository commit and always links the mock backend (no real-liburma detection yet), and that urma_performance has no Bazel target.
bazel/third_party/umdk only contained umdk.BUILD, unlike every other bazel/third_party/<dep> directory which pairs its <dep>.BUILD with an empty BUILD.bazel marking the directory as a package. Without it, resolving the git_repository(build_file = "//bazel/third_party/umdk:umdk.BUILD") label fails once the umdk repo is actually fetched: Error in read: Unable to load package for //bazel/third_party/umdk:umdk.BUILD: BUILD file not found ... This broke `bazel build --define=BRPC_WITH_URMA=true //:brpc` on both x86_64 and arm64 (confirmed via GitHub Actions CI on ubuntu-22.04 and ubuntu-24.04-arm).
…works The umdk repo ships its own src/urma/BUILD.bazel, which makes Bazel treat src/urma/ as a separate package. glob() in our umdk.BUILD (rooted at the umdk repo root) can't cross that boundary, so hdrs = glob(["src/urma/lib/urma/**/include/*.h"]) silently resolved to an empty list. The cc_library then exported no header inputs, so compiling anything that #includes urma_api.h failed with "No such file or directory" even though the -isystem path was correct and the file existed on disk. Confirmed via GitHub Actions on both ubuntu-22.04 (x86_64) and ubuntu-24.04-arm (arm64): `bazel build --define=BRPC_WITH_URMA=true //:brpc` failed identically on both before this patch_cmds fix.
Reviewer dwh110 pointed out that src/brpc/urma/mock_urma.cpp had no independent feature switch: whenever liburma wasn't found, CMake/Make silently linked the mock, which can produce a binary that looks URMA-capable but can't reach real hardware. Add WITH_URMA_MOCK (CMake) / --with-urma-mock (config_brpc.sh), default OFF. When WITH_URMA is enabled and liburma isn't found, the build now fails with a clear message unless the mock is explicitly requested, instead of substituting it implicitly. Document the new flag in docs/en/urma.md and docs/cn/urma.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add conditional linking for URMA library based on availability.
Added a comment regarding linking with liburma based on brpc build options.
Added logic to find the URMA library and handle its absence.
Removed conditional check for URMA library and related messages.
Update logic for linking liburma based on header presence.
cw20050111-prog
force-pushed
the
urma_transport
branch
from
August 19, 2026 10:13
f3b3d3c to
7949cea
Compare
Author
|
Superseded — replacing with a rebased (linear history, no merge commit) version of this branch. Will open a new PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch (cw20050111-prog/brpc:urma_transport) is a continuation of this repo's
urma_transportbranch, prepared to update the community PR apache#3428.What's included on top of the current
urma_transportHEAD (949eba49)apache/brpc:master(26 commits), which pulls in therpcz root client span lifetimefix (apache/brpc@cb84e83c) — this resolves theclang-unittest-asanLeakSanitizer failure that was blocking feat: add URMA transport support apache/brpc#3428's CIVerified
Please review and merge into
urma_transporton your fork when convenient; you can then push/update apache#3428 from there.