wwbmmm commented on code in PR #3197: URL: https://github.com/apache/brpc/pull/3197#discussion_r2749164098
########## src/brpc/policy/flatbuffers_protocol.cpp: ########## @@ -0,0 +1,472 @@ +// 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 "butil/logging.h" // LOG() +#include "butil/iobuf.h" // butil::IOBuf +#include "butil/single_iobuf.h" // butil::SingleIOBuf +#include "butil/time.h" + +#include "butil/raw_pack.h" // RawPacker RawUnpacker + +#include "brpc/controller.h" // Controller +#include "brpc/socket.h" // Socket +#include "brpc/server.h" // Server +#include "brpc/stream_impl.h" +#include "brpc/rpc_dump.h" // SampledRequest +#include "brpc/policy/most_common_message.h" +#include "brpc/details/controller_private_accessor.h" +#include "brpc/details/server_private_accessor.h" +#include "brpc/policy/flatbuffers_protocol.h" + +namespace brpc { +namespace policy { + +struct FBRpcRequestMeta { + struct { + uint32_t service_index; + int32_t method_index; + } request; + int32_t message_size; + int32_t attachment_size; + int64_t correlation_id; +}__attribute__((packed)); + +struct FBRpcResponseMeta { + struct { + int32_t error_code; + } response; + int32_t message_size; + int32_t attachment_size; + int64_t correlation_id; +}__attribute__((packed)); + +struct FBRpcRequestHeader { + char header[12]; + struct FBRpcRequestMeta meta; +}__attribute__((packed)); + +struct FBRpcResponseHeader { + char header[12]; + struct FBRpcResponseMeta meta; +}__attribute__((packed)); + +bool inline ParseFbFromIOBuf(brpc::flatbuffers::Message* msg, size_t msg_size, const butil::IOBuf& buf) { + return brpc::flatbuffers::ParseFbFromIOBUF(msg, msg_size, buf); +} + +// Notes: +// 1. 12-byte header [BRPC][body_size][meta_size] +// 2. body_size and meta_size are in network byte order +// 3. Use service->service_index + method_index to specify the method to call +// 4. `attachment_size' is set iff request/response has attachment +// 5. Not supported: chunk_info + +// Pack header into `buf' + +static inline void PackFlatbuffersRpcHeader(char* rpc_header, int meta_size, int payload_size) { + // supress strict-aliasing warning. + uint32_t* dummy = (uint32_t*)rpc_header; + *dummy = *(uint32_t*)"BRPC"; + butil::RawPacker(rpc_header + 4) + .pack32(meta_size + payload_size) + .pack32(meta_size); +} + +static inline bool ParseMetaBufferFromIOBUF(butil::SingleIOBuf* dest, + const butil::IOBuf& source, uint32_t msg_size) { + return dest->assign(source, msg_size); +} + +ParseResult ParseFlatBuffersMessage(butil::IOBuf* source, Socket* socket, + bool /*read_eof*/, const void*) { + char header_buf[12]; + const size_t n = source->copy_to(header_buf, sizeof(header_buf)); + if (n >= 4) { + void* dummy = header_buf; + if (*(const uint32_t*)dummy != *(const uint32_t*)"BRPC") { + return MakeParseError(PARSE_ERROR_TRY_OTHERS); + } + } else { + if (memcmp(header_buf, "BRPC", n) != 0) { Review Comment: BRPC这个名字改一下，比如FRPC？ ########## src/brpc/policy/flatbuffers_protocol.cpp: ########## @@ -0,0 +1,472 @@ +// 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 "butil/logging.h" // LOG() +#include "butil/iobuf.h" // butil::IOBuf +#include "butil/single_iobuf.h" // butil::SingleIOBuf +#include "butil/time.h" + +#include "butil/raw_pack.h" // RawPacker RawUnpacker + +#include "brpc/controller.h" // Controller +#include "brpc/socket.h" // Socket +#include "brpc/server.h" // Server +#include "brpc/stream_impl.h" +#include "brpc/rpc_dump.h" // SampledRequest +#include "brpc/policy/most_common_message.h" +#include "brpc/details/controller_private_accessor.h" +#include "brpc/details/server_private_accessor.h" +#include "brpc/policy/flatbuffers_protocol.h" + +namespace brpc { +namespace policy { + +struct FBRpcRequestMeta { + struct { + uint32_t service_index; + int32_t method_index; + } request; + int32_t message_size; + int32_t attachment_size; + int64_t correlation_id; +}__attribute__((packed)); + +struct FBRpcResponseMeta { + struct { + int32_t error_code; + } response; + int32_t message_size; + int32_t attachment_size; + int64_t correlation_id; +}__attribute__((packed)); + +struct FBRpcRequestHeader { + char header[12]; + struct FBRpcRequestMeta meta; +}__attribute__((packed)); + +struct FBRpcResponseHeader { + char header[12]; + struct FBRpcResponseMeta meta; +}__attribute__((packed)); + +bool inline ParseFbFromIOBuf(brpc::flatbuffers::Message* msg, size_t msg_size, const butil::IOBuf& buf) { + return brpc::flatbuffers::ParseFbFromIOBUF(msg, msg_size, buf); +} + +// Notes: +// 1. 12-byte header [BRPC][body_size][meta_size] +// 2. body_size and meta_size are in network byte order +// 3. Use service->service_index + method_index to specify the method to call +// 4. `attachment_size' is set iff request/response has attachment +// 5. Not supported: chunk_info + +// Pack header into `buf' + +static inline void PackFlatbuffersRpcHeader(char* rpc_header, int meta_size, int payload_size) { + // supress strict-aliasing warning. + uint32_t* dummy = (uint32_t*)rpc_header; + *dummy = *(uint32_t*)"BRPC"; + butil::RawPacker(rpc_header + 4) + .pack32(meta_size + payload_size) + .pack32(meta_size); +} + +static inline bool ParseMetaBufferFromIOBUF(butil::SingleIOBuf* dest, + const butil::IOBuf& source, uint32_t msg_size) { + return dest->assign(source, msg_size); +} + +ParseResult ParseFlatBuffersMessage(butil::IOBuf* source, Socket* socket, + bool /*read_eof*/, const void*) { + char header_buf[12]; + const size_t n = source->copy_to(header_buf, sizeof(header_buf)); + if (n >= 4) { + void* dummy = header_buf; + if (*(const uint32_t*)dummy != *(const uint32_t*)"BRPC") { + return MakeParseError(PARSE_ERROR_TRY_OTHERS); + } + } else { + if (memcmp(header_buf, "BRPC", n) != 0) { + return MakeParseError(PARSE_ERROR_TRY_OTHERS); + } + } + if (n < sizeof(header_buf)) { Review Comment: 适当加一些空行，代码看上去太挤了 ########## src/brpc/protocol.h: ########## @@ -101,7 +101,7 @@ struct Protocol { typedef void (*SerializeRequest)( butil::IOBuf* request_buf, Controller* cntl, - const google::protobuf::Message* request); + const void* request_obj); Review Comment: 这个改动影响太大了，我认为是不可接受的。不仅是brpc代码库中的所有protocol实现需要修改，而且其他用户自己实现的protocol也要修改（即不兼容升级） 我提一个思路你参考一下：你可以让brpc::flatbuffers::Message继承brpc::NonreflectableMessage，这样这个message就自动继承了google::protobuf::Message，这样就可以在不修改brpc框架接口的前提下扩展出flatbuffer协议。你可以参考brpc Redis/Memcached等协议的实现，这些协议实际上并不用到protobuf，但也保持了google::protobuf::Message的接口形式。 -- 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]
