Copilot commented on code in PR #3428: URL: https://github.com/apache/brpc/pull/3428#discussion_r3743002447
########## src/brpc/urma/urma_helper.cpp: ########## @@ -0,0 +1,788 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "brpc/urma/urma_helper.h" + +#if BRPC_WITH_URMA + +#include <errno.h> +#include <pthread.h> +#include <stdlib.h> +#include <string.h> +#include <sys/mman.h> +#include <unistd.h> + +#include <atomic> +#include <new> +#include <utility> +#include <vector> + +#include <gflags/gflags.h> + +#include "butil/atomicops.h" +#include "butil/containers/flat_map.h" +#include "butil/iobuf.h" +#include "butil/logging.h" Review Comment: This file hard-codes the IOBuf block header length as 32 bytes, but the actual overhead is sizeof(butil::IOBuf::Block). Hard-coding makes URMA buffer sizing fragile (e.g., if IOBuf::Block layout changes or on non-64bit builds). Consider including the inline definition so the size can be derived from sizeof(IOBuf::Block). This issue also appears on line 103 of the same file. ########## test/brpc_urma_unittest.cpp: ########## @@ -0,0 +1,609 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <cstring> +#include <gtest/gtest.h> +#include <gflags/gflags.h> + +#if BRPC_WITH_URMA +#include "butil/atomicops.h" +#include "butil/sys_byteorder.h" +#include "urma_api.h" +#include "brpc/urma/urma_handshake.h" +#include "brpc/urma/urma_handshake.pb.h" +#include "brpc/urma/urma_helper.h" +#include "urma_types.h" + +using namespace brpc; + +namespace brpc { +namespace urma { + +DECLARE_int32(urma_client_handshake_version); +extern bool g_skip_urma_init; +extern butil::atomic<bool> g_urma_available; + +} // namespace urma +} // namespace brpc + +// --------------------------------------------------------------------------- +// v2 binary HelloMessage: serialize + deserialize round-trips. +// --------------------------------------------------------------------------- +TEST(UrmaHandshakeTest, v2_serialize_deserialize_roundtrip) { + urma::v2_wire::HelloMessage m; + m.msg_len = urma::v2_wire::HELLO_PACKET_LEN; + m.hello_ver = urma::v2_wire::HELLO_V2_VERSION; + m.impl_ver = urma::v2_wire::IMPL_V2_VERSION; + m.buffer_size = 8192; + m.recv_buffer_cnt = 127; + m.jetty_id = 0x12345678; + for (int i = 0; i < 16; ++i) { + m.eid[i] = static_cast<uint8_t>(i + 1); + } + m.uasid = 0xdeadbeef; + m.tp_type = 1; // URMA_CTP + for (int i = 0; i < 16; ++i) { + m.seg_eid[i] = static_cast<uint8_t>(16 - i); + } + m.seg_uasid = 0xcafebabe; + m.seg_va = 0x1122334455667788ULL; + m.seg_len = 1ULL << 20; + m.seg_token_id = 0x42424242; + + uint8_t buf[urma::v2_wire::HELLO_BODY_LEN]; + m.Serialize(buf); + + urma::v2_wire::HelloMessage m2; + m2.Deserialize(buf); + EXPECT_EQ(m.msg_len, m2.msg_len); + EXPECT_EQ(m.hello_ver, m2.hello_ver); + EXPECT_EQ(m.impl_ver, m2.impl_ver); + EXPECT_EQ(m.buffer_size, m2.buffer_size); + EXPECT_EQ(m.recv_buffer_cnt, m2.recv_buffer_cnt); + EXPECT_EQ(m.jetty_id, m2.jetty_id); + EXPECT_EQ(0, memcmp(m.eid, m2.eid, 16)); + EXPECT_EQ(m.uasid, m2.uasid); + EXPECT_EQ(m.tp_type, m2.tp_type); + EXPECT_EQ(0, memcmp(m.seg_eid, m2.seg_eid, 16)); + EXPECT_EQ(m.seg_uasid, m2.seg_uasid); + EXPECT_EQ(m.seg_va, m2.seg_va); + EXPECT_EQ(m.seg_len, m2.seg_len); + EXPECT_EQ(m.seg_token_id, m2.seg_token_id); +} + +// --------------------------------------------------------------------------- +// v2 packet on the wire: "URMA" magic + body. +// --------------------------------------------------------------------------- +TEST(UrmaHandshakeTest, v2_packet_magic_is_urma) { + EXPECT_EQ(4u, urma::v2_wire::MAGIC_STR_LEN); + char magic[4] = {'U', 'R', 'M', 'A'}; + EXPECT_EQ(0, memcmp(magic, "URMA", 4)); + EXPECT_EQ(4u + 82u, urma::v2_wire::HELLO_PACKET_LEN); +} + +// --------------------------------------------------------------------------- +// v3 protobuf UrmaHello: serialize + parse round-trips. +// --------------------------------------------------------------------------- +TEST(UrmaHandshakeTest, v3_protobuf_roundtrip) { + urma::UrmaHello msg; + msg.set_buffer_size(8192); + msg.set_recv_buffer_cnt(127); + msg.set_jetty_id(0x12345678); + uint8_t eid[16]; + for (int i = 0; i < 16; ++i) { + eid[i] = static_cast<uint8_t>(i + 1); + } + msg.set_eid(eid, 16); + msg.set_uasid(0xdeadbeef); + msg.set_tp_type(1); + uint8_t seg_eid[16]; + for (int i = 0; i < 16; ++i) { + seg_eid[i] = static_cast<uint8_t>(16 - i); + } + msg.set_seg_eid(seg_eid, 16); + msg.set_seg_uasid(0xcafebabe); + msg.set_seg_va(0x1122334455667788ULL); + msg.set_seg_len(1ULL << 20); + msg.set_seg_token_id(0x42424242); + + std::string body; + ASSERT_TRUE(msg.SerializeToString(&body)); + urma::UrmaHello msg2; + ASSERT_TRUE(msg2.ParseFromString(body)); + EXPECT_EQ(msg.buffer_size(), msg2.buffer_size()); + EXPECT_EQ(msg.recv_buffer_cnt(), msg2.recv_buffer_cnt()); + EXPECT_EQ(msg.jetty_id(), msg2.jetty_id()); + EXPECT_EQ(16, msg2.eid().size()); + EXPECT_EQ(0, memcmp(msg.eid().data(), msg2.eid().data(), 16)); + EXPECT_EQ(msg.uasid(), msg2.uasid()); + EXPECT_EQ(msg.tp_type(), msg2.tp_type()); + EXPECT_EQ(16, msg2.seg_eid().size()); + EXPECT_EQ(msg.seg_uasid(), msg2.seg_uasid()); + EXPECT_EQ(msg.seg_va(), msg2.seg_va()); + EXPECT_EQ(msg.seg_len(), msg2.seg_len()); + EXPECT_EQ(msg.seg_token_id(), msg2.seg_token_id()); +} + +// --------------------------------------------------------------------------- +// CreateServerHandshakeByMagic dispatches on the magic bytes. +// --------------------------------------------------------------------------- +TEST(UrmaHandshakeTest, server_handshake_factory_dispatches_on_magic) { + // We cannot fully exercise the server handshake without a real socket + + // endpoint, but we can verify the factory returns the right protocol + // version for each magic, and nullptr for an unknown magic. + uint8_t magic_v2[4] = {'U', 'R', 'M', 'A'}; + uint8_t magic_v3[4] = {'U', 'R', 'M', '3'}; + uint8_t magic_bad[4] = {'P', 'R', 'P', 'C'}; + + // v2 magic -> protocol version 2 + urma::UrmaHandshake* hs2 = + urma::CreateServerHandshakeByMagic(nullptr, magic_v2); + // Note: the factory dereferences the endpoint only inside SendLocalHello / + // ReceiveAndParseRemoteHello; passing nullptr is safe for the version query. + // (We delete immediately to avoid touching the endpoint.) + if (hs2) { + EXPECT_EQ(2, hs2->ProtocolVersion()); + delete hs2; + } + // v3 magic -> protocol version 3 + urma::UrmaHandshake* hs3 = + urma::CreateServerHandshakeByMagic(nullptr, magic_v3); + if (hs3) { + EXPECT_EQ(3, hs3->ProtocolVersion()); + delete hs3; + } + // unknown magic -> nullptr (caller falls back to TCP) + urma::UrmaHandshake* hsb = + urma::CreateServerHandshakeByMagic(nullptr, magic_bad); + EXPECT_EQ(nullptr, hsb); +} + +// --------------------------------------------------------------------------- +// CreateClientHandshake picks the version from the gflag. +// --------------------------------------------------------------------------- +TEST(UrmaHandshakeTest, client_handshake_factory_respects_flag) { + const int saved = urma::FLAGS_urma_client_handshake_version; + + urma::FLAGS_urma_client_handshake_version = 2; + urma::UrmaHandshake* hs2 = urma::CreateClientHandshake(nullptr); + if (hs2) { + EXPECT_EQ(2, hs2->ProtocolVersion()); + delete hs2; + } + + urma::FLAGS_urma_client_handshake_version = 3; + urma::UrmaHandshake* hs3 = urma::CreateClientHandshake(nullptr); + if (hs3) { + EXPECT_EQ(3, hs3->ProtocolVersion()); + delete hs3; + } + + urma::FLAGS_urma_client_handshake_version = saved; +} + +// --------------------------------------------------------------------------- +// 4-byte ACK: HELLO_ACK_URMA_OK bit. +// --------------------------------------------------------------------------- +TEST(UrmaHandshakeTest, ack_bit_is_rdma_ok) { Review Comment: The test case name refers to RDMA even though it is validating URMA ACK semantics, which makes the test suite misleading and harder to search/maintain. Rename the test to use "urma" consistently. ########## WORKSPACE: ########## @@ -279,6 +279,13 @@ http_archive( urls = ["https://archive.apache.org/dist/thrift/0.15.0/thrift-0.15.0.tar.gz";], ) +git_repository( + name = "umdk", + build_file = "//bazel/third_party/umdk:umdk.BUILD", + remote = "https://atomgit.com/openeuler/umdk.git";, + tag = "v26.06.0_CAM", +) Review Comment: This Bazel dependency is pinned by a movable tag (git_repository + tag). For hermetic/reproducible builds it should be pinned to an immutable commit SHA (like boringssl above) so the same BRPC revision always resolves the same UMDK sources. ########## MODULE.bazel: ########## @@ -52,3 +52,14 @@ git_override( remote = 'https://github.com/hedronvision/bazel-compile-commands-extractor.git', commit = '1e08f8e0507b6b6b1f4416a9a22cf5c28beaba93', # Jun 28, 2024 ) + +git_repository = use_repo_rule( + '@bazel_tools//tools/build_defs/repo:git.bzl', + 'git_repository', +) +git_repository( + name = 'umdk', + build_file = '//bazel/third_party/umdk:umdk.BUILD', + remote = 'https://atomgit.com/openeuler/umdk.git', + tag = 'v26.06.0_CAM', +) Review Comment: MODULE.bazel also pulls UMDK via a movable tag. Pinning to a commit (or another immutable reference) would make bzlmod resolution reproducible across time and mirrors. -- 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]
