[GitHub] [incubator-brpc] zzjshaok opened a new issue #992: ParallelChannel发送消息偶尔失败
zzjshaok opened a new issue #992: ParallelChannel发送消息偶尔失败 URL: https://github.com/apache/incubator-brpc/issues/992 brpc::ParallelChannel 对象内部只AddChannel了一个sub channel,进行rpc 客户端调用时,有时候会报错,业务代码如下: stub.rnd_next(&(m_readrpcDone->cntl), &request, &(m_readrpcDone->read_response), m_readrpcDone); if (m_readrpcDone->cntl.Failed()) { MLOG_ERROR("errcode %d, errmsg %s,remote_side %s, local_side %s", m_readrpcDone->cntl.ErrorCode(), m_readrpcDone->cntl.ErrorText().c_str(), butil::endpoint2str(m_readrpcDone->cntl.remote_side()).c_str(),butil::endpoint2str(m_readrpcDone->cntl.local_side()).c_str()); return m_readrpcDone->cntl.ErrorCode(); } 调用失败时,会进入m_readrpcDone->cntl.Failed()内部,此时打印相关信息如下: (gdb) p m_readrpcDone->cntl $1 = { = {_vptr.RpcController = 0x7f5212d22df0 }, static FLAGS_IGNORE_EOVERCROWDED = 1, static FLAGS_SECURITY_MODE = 2, static FLAGS_ADDED_CONCURRENCY = 4, static FLAGS_READ_PROGRESSIVELY = 8, static FLAGS_PROGRESSIVE_READER = 16, static FLAGS_BACKUP_REQUEST = 32, static FLAGS_DESTROY_CID_IN_DONE = 128, static FLAGS_CLOSE_CONNECTION = 256, static FLAGS_LOG_ID = 512, static FLAGS_REQUEST_CODE = 1024, static FLAGS_PB_BYTES_TO_BASE64 = 2048, static FLAGS_ALLOW_DONE_TO_RUN_IN_PLACE = 4096, static FLAGS_USED_BY_RPC = 8192, static FLAGS_REQUEST_WITH_AUTH = 32768, static FLAGS_PB_JSONIFY_EMPTY_ARRAY = 65536, static FLAGS_ENABLED_CIRCUIT_BREAKER = 131072, static FLAGS_ALWAYS_PRINT_PRIMITIVE_FIELDS = 262144, _span = 0x0, _flags = 10368, _error_code = 1008, _error_text = {static npos = 18446744073709551615, _M_dataplus = {> = {<__gnu_cxx::new_allocator> = {}, }, _M_p = 0x7f51b9259f60 "[E1008][HandleSocketFailed@/data01/zhangzj/mdb_4.0/mariadb-10.4.8/storage/mdb/src/3rd/brpc/src/brpc/controller.cpp:1194]Reached timeout=5ms @0.0.0.0:0"}, _M_string_length = 154, { _M_local_buf = "\234\000\000\000\000\000\000\000Handle\000", _M_allocated_capacity = 156}}, _remote_side = {ip = {s_addr = 0}, port = 0}, _local_side = {ip = {s_addr = 0}, port = 0}, _session_local_data = 0x0, _server = 0x0, _oncancel_id = {value = 0}, _auth_context = 0x0, _mongo_session_data = {px = 0x0}, _rpc_dump_meta = 0x0, _request_protocol = brpc::PROTOCOL_UNKNOWN, _max_retry = -123456789, _retry_policy = 0x0, _correlation_id = {value = 7696581394433}, _connection_type = brpc::CONNECTION_TYPE_UNKNOWN, _fail_limit = -123456789, _pipelined_count = 0, _timeout_ms = 5, _connect_timeout_ms = -123456789, _backup_request_ms = -123456789, _deadline_us = 1576054482870493, _timeout_id = 0, _begin_time_us = 1576054432870493, _end_time_us = 1576054482871014, _tos = 0, _preferred_index = -1, _request_compress_type = brpc::COMPRESS_TYPE_NONE, _response_compress_type = brpc::COMPRESS_TYPE_NONE, _log_id = 0, _pchan_sub_count = 1, _response = 0x7f5178c64f48, _done = 0x7f5175355a80, _sender = 0x0, _request_code = 0, _single_server_id = 18446744073709551615, _lb = {px = 0x0}, _tmp_completion_info = {id = {value = 7696581394433}, responded = false}, _current_call = {nretry = 0, need_feedback = false, enable_circuit_breaker = false, touched_by_stream_creator = false, peer_id = 18446744073709551615, begin_time_us = 0, sending_sock = { _M_t = {> = {> = {> = { = {}, }, }, > = {_M_head_impl = 0x0}, }, }}, stream_user_data = 0x0}, _unfinished_call = 0x0, _accessed = 0x0, _stream_creator = 0x0, _pack_request = 0x0, _method = 0x0, _auth = 0x0, _request_buf = { static DEFAULT_BLOCK_SIZE = 8192, static INITIAL_CAP = 32, static INVALID_AREA = 0, {_bv = {magic = 0, start = 0, refs = 0x0, nref = 0, cap_mask = 0, nbytes = 0}, _sv = {refs = {{offset = 0, length = 0, block = 0x0}, {offset = 0, length = 0, block = 0x0}, _idl_names = {request_name = 0x7f5212716d6b "req", response_name = 0x7f521272da73 "res"}, ---Type to continue, or q to quit--- _idl_result = 12345678987654321, _http_request = 0x0, _http_response = 0x0, _request_attachment = { static DEFAULT_BLOCK_SIZE = 8192, static INITIAL_CAP = 32, static INVALID_AREA = 0, {_bv = {magic = 0, start = 0, refs = 0x0, nref = 0, cap_mask = 0, nbytes = 0}, _sv = {refs = {{offset = 0, length = 0, block = 0x0}, {offset = 0, length = 0, block = 0x0}, _response_attachment = {static DEFAULT_BLOCK_SIZE = 8192, static INITIAL_CAP = 32, static INVALID_AREA = 0, {_bv = {magic = 0, start = 0, refs = 0x0, nref = 0, cap_mask = 0, nbytes = 0}, _sv = {refs = {{ offset = 0, length = 0, block = 0x0}, {offset = 0, length = 0, block = 0x0}, _wpa = {px = 0x0}, _rpa = {px = 0x0}, _request_stream = 18446744073709551615, _response_stream = 18446744073709551615, _remote_stream_settings = 0x0, _thrift_method_name = {static npos = 18446744073709551615, _M_dataplus = {> = {<_
committer 申请 -- 何磊
Hi all: 大家好,我是何磊, 来自爱奇艺广告销售部,目前主要负责广告引擎基础架构的相关工作,github主页是 https://github.com/TousakaRin 。我们团队在 2017 年引入了 brpc,并对广告引擎大部分后台服务进行了brpc改造。我们在爱奇艺内部维护了自己分支,并且会定期和社区同步代码,同时也会将一些 feature 及 bug fix 推送到社区。期间我们向社区推送了consul名字服务、自动限流、单节点熔断,multi连接方式等 feature 以及一些 bug fix。其中自动限流及单节点熔断功能均主要由我开发并且已经merge到master分支。 接下来我希望能够更进一步的参与到 brpc 的开发与推广工作中,包括但不限于 1. 参与发版/技术路线的讨论。 2. bug 修复及 提供feature。 3. brpc的推广。
Re: committer 申请 -- 何磊
thanks helei, we will discuss it and call for a vote later, according to apache rules. Let me translate your request into English for better understanding as below. "Hello, I'm he Lei from iqiyi advertising department. I'm mainly responsible for the infrastructure of advertising engine. My home page of GitHub is https://github.com/tousakarin. My team introduced brpc in 2017, and adopted brpc for most of the background services of advertising engine. We maintain our own branch within iqiyi, and regularly synchronize code with the community, and also push some features and bug fix to the community. During this period, I pushed features such as consumer name service, automatic current limiting, single node fusing, multi connection mode and some bug fix to the community. The automatic current limiting and single node fusing functions are mainly developed by me and have been merged to the master branch. Next, I hope to further participate in the development and promotion of brpc, including but not limited to 1. Participate in the discussion of distribution / technical route. 2. Bug fix and provide features. 3. The promotion of brpc" 在 2019/12/11 下午6:41,“lhestz” 写入: Hi all: 大家好,我是何磊, 来自爱奇艺广告销售部,目前主要负责广告引擎基础架构的相关工作,github主页是 https://github.com/TousakaRin 。我们团队在 2017 年引入了 brpc,并对广告引擎大部分后台服务进行了brpc改造。我们在爱奇艺内部维护了自己分支,并且会定期和社区同步代码,同时也会将一些 feature 及 bug fix 推送到社区。期间我们向社区推送了consul名字服务、自动限流、单节点熔断,multi连接方式等 feature 以及一些 bug fix。其中自动限流及单节点熔断功能均主要由我开发并且已经merge到master分支。 接下来我希望能够更进一步的参与到 brpc 的开发与推广工作中,包括但不限于 1. 参与发版/技术路线的讨论。 2. bug 修复及 提供feature。 3. brpc的推广。
vote to accept helei as committer
Hi, guys, Let us vote to accept Helei as committer. Please reply +1 if you agree, Reply -1 if you disagree, together with your reason. Thanks Below is his application: "Hello, I'm he Lei from iqiyi advertising department. I'm mainly responsible for the infrastructure of advertising engine. My home page of GitHub is https://github.com/tousakarin. My team introduced brpc in 2017, and adopted brpc for most of the background services of advertising engine. We maintain our own branch within iqiyi, and regularly synchronize code with the community, and also push some features and bug fix to the community. During this period, I pushed features such as consumer name service, automatic current limiting, single node fusing, multi connection mode and some bug fix to the community. The automatic current limiting and single node fusing functions are mainly developed by me and have been merged to the master branch. Next, I hope to further participate in the development and promotion of brpc, including but not limited to 1. Participate in the discussion of distribution / technical route. 2. Bug fix and provide features. 3. The promotion of brpc"
Re: vote to accept helei as committer
+1. tan zhongyi 于2019年12月11日周三 下午11:26写道: > Hi, guys, > > Let us vote to accept Helei as committer. > Please reply +1 if you agree, > Reply -1 if you disagree, together with your reason. > Thanks > > > Below is his application: > > "Hello, I'm he Lei from iqiyi advertising department. I'm mainly > responsible for the infrastructure of advertising engine. My home page of > GitHub is https://github.com/tousakarin. My team introduced brpc in 2017, > and adopted brpc for most of the background services of advertising engine. > We maintain our own branch within iqiyi, and regularly synchronize code > with the community, and also push some features and bug fix to the > community. During this period, I pushed features such as consumer name > service, automatic current limiting, single node fusing, multi connection > mode and some bug fix to the community. The automatic current limiting and > single node fusing functions are mainly developed by me and have been > merged to the master branch. > > Next, I hope to further participate in the development and promotion of > brpc, including but not limited to 1. Participate in the discussion of > distribution / technical route. 2. Bug fix and provide features. 3. The > promotion of brpc" > > > -- Jiashun Zhu
Re: vote to accept helei as committer
+1 On Thu, Dec 12, 2019 at 11:12 AM JiashunZhu wrote: > +1. > > tan zhongyi 于2019年12月11日周三 下午11:26写道: > > > Hi, guys, > > > > Let us vote to accept Helei as committer. > > Please reply +1 if you agree, > > Reply -1 if you disagree, together with your reason. > > Thanks > > > > > > Below is his application: > > > > "Hello, I'm he Lei from iqiyi advertising department. I'm mainly > > responsible for the infrastructure of advertising engine. My home page of > > GitHub is https://github.com/tousakarin. My team introduced brpc in > 2017, > > and adopted brpc for most of the background services of advertising > engine. > > We maintain our own branch within iqiyi, and regularly synchronize code > > with the community, and also push some features and bug fix to the > > community. During this period, I pushed features such as consumer name > > service, automatic current limiting, single node fusing, multi connection > > mode and some bug fix to the community. The automatic current limiting > and > > single node fusing functions are mainly developed by me and have been > > merged to the master branch. > > > > Next, I hope to further participate in the development and promotion of > > brpc, including but not limited to 1. Participate in the discussion of > > distribution / technical route. 2. Bug fix and provide features. 3. The > > promotion of brpc" > > > > > > > > -- > Jiashun Zhu >
Re: vote to accept helei as committer
+1 On Thu, Dec 12, 2019 at 11:17 AM James Ge wrote: > +1 > > On Thu, Dec 12, 2019 at 11:12 AM JiashunZhu > wrote: > > > +1. > > > > tan zhongyi 于2019年12月11日周三 下午11:26写道: > > > > > Hi, guys, > > > > > > Let us vote to accept Helei as committer. > > > Please reply +1 if you agree, > > > Reply -1 if you disagree, together with your reason. > > > Thanks > > > > > > > > > Below is his application: > > > > > > "Hello, I'm he Lei from iqiyi advertising department. I'm mainly > > > responsible for the infrastructure of advertising engine. My home page > of > > > GitHub is https://github.com/tousakarin. My team introduced brpc in > > 2017, > > > and adopted brpc for most of the background services of advertising > > engine. > > > We maintain our own branch within iqiyi, and regularly synchronize code > > > with the community, and also push some features and bug fix to the > > > community. During this period, I pushed features such as consumer name > > > service, automatic current limiting, single node fusing, multi > connection > > > mode and some bug fix to the community. The automatic current limiting > > and > > > single node fusing functions are mainly developed by me and have been > > > merged to the master branch. > > > > > > Next, I hope to further participate in the development and promotion of > > > brpc, including but not limited to 1. Participate in the discussion of > > > distribution / technical route. 2. Bug fix and provide features. 3. The > > > promotion of brpc" > > > > > > > > > > > > > -- > > Jiashun Zhu > > >
[GitHub] [incubator-brpc] lpstudy opened a new issue #993: Does IOPortal consider to support create memory aligned block for direct IO
lpstudy opened a new issue #993: Does IOPortal consider to support create memory aligned block for direct IO URL: https://github.com/apache/incubator-brpc/issues/993 1. IOPortal calls `pappend_from_file_descriptor` to read data from a opened `fd`, where in my condition `fd` is opened with `O_DIRECT` flag, but the function failed with Invalid arguments. I am sure that the offset and size are chunk-aligned, so I guess this is because the memory buffer is not chunk-aligned. 2. I read the iobuf.cpp, it call blockmem_allocate (default is ::malloc) to alloc memory for `create_block`, which is not memory-aligned for direct io operations. This may be the reason for invalid argument. 3. Do we have plan for memory-aligned block allocation or in deed is there a way to achieve this goal. I want to minimize the data copy as fewer as possible. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
申请成为brpc committer
hi,all 我是牟盖东, 来自爱奇艺广告部门,负责广告在线服务的架构。 2017年,我将brpc引入我们业务线,到2019年我们所有服务已经改造为基于brpc框架。期间,我提交过bug fix,并与2019年参加百度技术沙龙,分享brpc中熔断和限流的实现原理。 后面,希望能进一步参与brpc社区事务,参与代码review和宣传活动等。
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356971336 ## File path: src/brpc/policy/redis_protocol.cpp ## @@ -52,62 +58,206 @@ struct InputResponse : public InputMessageBase { } }; -// "Message" = "Response" as we only implement the client for redis. -ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket, - bool /*read_eof*/, const void* /*arg*/) { -if (source->empty()) { -return MakeParseError(PARSE_ERROR_NOT_ENOUGH_DATA); +// This class is as parsing_context in socket. +class RedisConnContext : public Destroyable { +public: +RedisConnContext() +: redis_service(NULL) +, handler_continue(NULL) {} +~RedisConnContext(); +// @Destroyable +void Destroy() override; + +int Init(); + +SocketId socket_id; +RedisService* redis_service; +// If user starts a transaction, handler_continue indicates the +// first handler pointer that triggers the transaction. +RedisCommandHandler* handler_continue; +// The redis command are parsed and pushed into this queue +bthread::ExecutionQueueId queue; + +RedisCommandParser parser; +}; + +int ConsumeTask(RedisConnContext* ctx, std::string* command, butil::IOBuf* sendbuf) { +butil::Arena arena; +RedisReply output(&arena); +if (ctx->handler_continue) { +RedisCommandHandler::Result result = +ctx->handler_continue->Run(command->c_str(), &output); +if (result == RedisCommandHandler::OK) { +ctx->handler_continue = NULL; +} +} else { +std::string comm; +comm.reserve(8); +for (int i = 0; i < (int)command->size() && (*command)[i] != ' '; ++i) { +comm.push_back(std::tolower((*command)[i])); +} +RedisCommandHandler* ch = ctx->redis_service->FindCommandHandler(comm); +if (!ch) { +char buf[64]; +snprintf(buf, sizeof(buf), "ERR unknown command `%s`", comm.c_str()); +output.SetError(buf); +} else { +RedisCommandHandler::Result result = ch->Run(command->c_str(), &output); +if (result == RedisCommandHandler::CONTINUE) { +ctx->handler_continue = ch; +} +} +} +output.SerializeToIOBuf(sendbuf); +return 0; +} + +int Consume(void* ctx, bthread::TaskIterator& iter) { +RedisConnContext* qctx = static_cast(ctx); +if (iter.is_queue_stopped()) { +delete qctx; +return 0; +} +SocketUniquePtr s; +bool has_err = false; +if (Socket::Address(qctx->socket_id, &s) != 0) { +LOG(WARNING) << "Fail to address redis socket"; +has_err = true; } -// NOTE(gejun): PopPipelinedInfo() is actually more contended than what -// I thought before. The Socket._pipeline_q is a SPSC queue pushed before -// sending and popped when response comes back, being protected by a -// mutex. Previously the mutex is shared with Socket._id_wait_list. When -// 200 bthreads access one redis-server, ~1.5s in total is spent on -// contention in 10-second duration. If the mutex is separated, the time -// drops to ~0.25s. I further replaced PeekPipelinedInfo() with -// GivebackPipelinedInfo() to lock only once(when receiving response) -// in most cases, and the time decreases to ~0.14s. -PipelinedInfo pi; -if (!socket->PopPipelinedInfo(&pi)) { -LOG(WARNING) << "No corresponding PipelinedInfo in socket"; -return MakeParseError(PARSE_ERROR_TRY_OTHERS); -} - -do { -InputResponse* msg = static_cast(socket->parsing_context()); -if (msg == NULL) { -msg = new InputResponse; -socket->reset_parsing_context(msg); +Socket::WriteOptions wopt; +wopt.ignore_eovercrowded = true; +butil::IOBuf sendbuf; +for (; iter; ++iter) { +std::unique_ptr guard(*iter); +if (has_err) { +continue; } +ConsumeTask(qctx, *iter, &sendbuf); +// If there are too many tasks to execute, latency of the front +// responses will be increased by waiting the following tasks to +// be completed. To prevent this, if the current buf size is greater +// than FLAGS_redis_batch_flush_max_size, we just write the current +// buf first. +if ((int)sendbuf.size() >= FLAGS_redis_batch_flush_data_size) { +LOG_IF(WARNING, s->Write(&sendbuf, &wopt) != 0) +<< "Fail to send redis reply"; +} +} +if (!has_err && !sendbuf.empty()) { +LOG_IF(WARNING, s->Write(&sendbuf, &wopt) != 0) +<< "Fail to send redis reply"; +} +return 0; +} + +// == impl of RedisConnContext == -const int consume_count = (pi.with_auth ? 1 : pi.count); +RedisConnContext::~RedisConnContex
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356952231 ## File path: src/brpc/redis.h ## @@ -209,7 +213,54 @@ class RedisResponse : public ::google::protobuf::Message { std::ostream& operator<<(std::ostream& os, const RedisRequest&); std::ostream& operator<<(std::ostream& os, const RedisResponse&); -} // namespace brpc +class RedisCommandHandler; + +// Implement this class and assign an instance to ServerOption.redis_service +// to enable redis support. +class RedisService { +public: +typedef std::unordered_map> CommandMap; +virtual ~RedisService() {} + +// Call this function to register `handler` that can handle command `name`. +bool AddCommandHandler(const std::string& name, RedisCommandHandler* handler); + +// This function should be touched by user and used by brpc deverloper only. Review comment: should not be 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r355987152 ## File path: example/redis_c++/redis_server.cpp ## @@ -0,0 +1,128 @@ +// 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. +// +// A brpc based redis-server. Currently just implement set and +// get, but it's sufficient that you can get the idea how to +// implement brpc::RedisCommandHandler. + +#include +#include +#include +#include +#include +#include + +class RedisServiceImpl : public brpc::RedisService { +public: +bool Set(const std::string& key, const std::string& value) { +int slot = butil::crc32c::Value(key.c_str(), key.size()) % HashSlotNum; +_mutex[slot].lock(); +_map[slot][key] = value; +_mutex[slot].unlock(); +return true; +} + +bool Get(const std::string& key, std::string* value) { +int slot = butil::crc32c::Value(key.c_str(), key.size()) % HashSlotNum; +_mutex[slot].lock(); +auto it = _map[slot].find(key); +if (it == _map[slot].end()) { +_mutex[slot].unlock(); +return false; +} +*value = it->second; +_mutex[slot].unlock(); +return true; +} + +private: +const static int HashSlotNum = 32; Review comment: 这个符合编码规范么?要么是kHashSlotNum要么是HASH_SLOT_NUM 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356952596 ## File path: src/brpc/policy/redis_protocol.h ## @@ -33,6 +33,13 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket *socket, bool read_eo // Actions to a redis response. void ProcessRedisResponse(InputMessageBase* msg); +// Actions to a redis request, which is left unimplemented. +// All requests are processed in execution queue pushed in +// the parsing process. This function must be declared since +// server side will enable redis as a server side protocol Review comment: server side will enable -> since the server only enables 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356952885 ## File path: src/brpc/redis.h ## @@ -209,7 +213,54 @@ class RedisResponse : public ::google::protobuf::Message { std::ostream& operator<<(std::ostream& os, const RedisRequest&); std::ostream& operator<<(std::ostream& os, const RedisResponse&); -} // namespace brpc +class RedisCommandHandler; + +// Implement this class and assign an instance to ServerOption.redis_service +// to enable redis support. +class RedisService { +public: +typedef std::unordered_map> CommandMap; Review comment: 内部用shared_ptr但仍有函数返回裸指针,多半是有问题的。这里需要shared_ptr么?和service一致即可。 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356952989 ## File path: src/brpc/redis_command.cpp ## @@ -360,4 +360,86 @@ butil::Status RedisCommandByComponents(butil::IOBuf* output, return butil::Status::OK(); } +RedisCommandParser::RedisCommandParser() { +Reset(); +} + +ParseError RedisCommandParser::Parse(butil::IOBuf& buf) { Review comment: 应该叫Consume 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356953916 ## File path: src/brpc/policy/redis_protocol.cpp ## @@ -52,62 +58,206 @@ struct InputResponse : public InputMessageBase { } }; -// "Message" = "Response" as we only implement the client for redis. -ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket, - bool /*read_eof*/, const void* /*arg*/) { -if (source->empty()) { -return MakeParseError(PARSE_ERROR_NOT_ENOUGH_DATA); +// This class is as parsing_context in socket. +class RedisConnContext : public Destroyable { +public: +RedisConnContext() +: redis_service(NULL) +, handler_continue(NULL) {} +~RedisConnContext(); +// @Destroyable +void Destroy() override; + +int Init(); + +SocketId socket_id; +RedisService* redis_service; +// If user starts a transaction, handler_continue indicates the +// first handler pointer that triggers the transaction. +RedisCommandHandler* handler_continue; +// The redis command are parsed and pushed into this queue +bthread::ExecutionQueueId queue; + +RedisCommandParser parser; +}; + +int ConsumeTask(RedisConnContext* ctx, std::string* command, butil::IOBuf* sendbuf) { +butil::Arena arena; +RedisReply output(&arena); +if (ctx->handler_continue) { +RedisCommandHandler::Result result = +ctx->handler_continue->Run(command->c_str(), &output); +if (result == RedisCommandHandler::OK) { +ctx->handler_continue = NULL; +} +} else { +std::string comm; +comm.reserve(8); +for (int i = 0; i < (int)command->size() && (*command)[i] != ' '; ++i) { +comm.push_back(std::tolower((*command)[i])); +} +RedisCommandHandler* ch = ctx->redis_service->FindCommandHandler(comm); +if (!ch) { +char buf[64]; +snprintf(buf, sizeof(buf), "ERR unknown command `%s`", comm.c_str()); +output.SetError(buf); +} else { +RedisCommandHandler::Result result = ch->Run(command->c_str(), &output); +if (result == RedisCommandHandler::CONTINUE) { +ctx->handler_continue = ch; +} +} +} +output.SerializeToIOBuf(sendbuf); +return 0; +} + +int Consume(void* ctx, bthread::TaskIterator& iter) { +RedisConnContext* qctx = static_cast(ctx); +if (iter.is_queue_stopped()) { +delete qctx; +return 0; +} +SocketUniquePtr s; +bool has_err = false; +if (Socket::Address(qctx->socket_id, &s) != 0) { +LOG(WARNING) << "Fail to address redis socket"; +has_err = true; } -// NOTE(gejun): PopPipelinedInfo() is actually more contended than what -// I thought before. The Socket._pipeline_q is a SPSC queue pushed before -// sending and popped when response comes back, being protected by a -// mutex. Previously the mutex is shared with Socket._id_wait_list. When -// 200 bthreads access one redis-server, ~1.5s in total is spent on -// contention in 10-second duration. If the mutex is separated, the time -// drops to ~0.25s. I further replaced PeekPipelinedInfo() with -// GivebackPipelinedInfo() to lock only once(when receiving response) -// in most cases, and the time decreases to ~0.14s. -PipelinedInfo pi; -if (!socket->PopPipelinedInfo(&pi)) { -LOG(WARNING) << "No corresponding PipelinedInfo in socket"; -return MakeParseError(PARSE_ERROR_TRY_OTHERS); -} - -do { -InputResponse* msg = static_cast(socket->parsing_context()); -if (msg == NULL) { -msg = new InputResponse; -socket->reset_parsing_context(msg); +Socket::WriteOptions wopt; +wopt.ignore_eovercrowded = true; +butil::IOBuf sendbuf; +for (; iter; ++iter) { +std::unique_ptr guard(*iter); +if (has_err) { +continue; } +ConsumeTask(qctx, *iter, &sendbuf); +// If there are too many tasks to execute, latency of the front +// responses will be increased by waiting the following tasks to +// be completed. To prevent this, if the current buf size is greater +// than FLAGS_redis_batch_flush_max_size, we just write the current +// buf first. +if ((int)sendbuf.size() >= FLAGS_redis_batch_flush_data_size) { +LOG_IF(WARNING, s->Write(&sendbuf, &wopt) != 0) +<< "Fail to send redis reply"; +} +} +if (!has_err && !sendbuf.empty()) { +LOG_IF(WARNING, s->Write(&sendbuf, &wopt) != 0) +<< "Fail to send redis reply"; +} +return 0; +} + +// == impl of RedisConnContext == -const int consume_count = (pi.with_auth ? 1 : pi.count); +RedisConnContext::~RedisConnContex
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356953233 ## File path: src/brpc/redis_reply.cpp ## @@ -189,7 +245,7 @@ ParseError RedisReply::ConsumePartialIOBuf(butil::IOBuf& buf, butil::Arena* aren return PARSE_ERROR_ABSOLUTELY_WRONG; } for (int64_t i = 0; i < count; ++i) { -new (&subs[i]) RedisReply; +new (&subs[i]) RedisReply(NULL); Review comment: 不是有没参数版本么?难道arena=null时和无参数构造行为不一致? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356954111 ## File path: src/brpc/redis_command.h ## @@ -40,6 +40,28 @@ butil::Status RedisCommandByComponents(butil::IOBuf* buf, const butil::StringPiece* components, size_t num_components); +// A parser used to parse redis raw command. +class RedisCommandParser { +public: +RedisCommandParser(); + +// Parse raw message from `buf'. Return PARSE_OK if successful. +ParseError Parse(butil::IOBuf& buf); + +// After Parse returns PARSE_OK, call this function to swap +// the parsed command string to `out'. +void SwapCommandTo(std::string* out); Review comment: 这种一次性的接口(只能swap一次)大概率是错误的设计。 就这个case,我觉得out应该是Parse的参数,且只会在返回PARSE_OK后被赋值。 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356953434 ## File path: src/brpc/redis_reply.cpp ## @@ -37,6 +38,61 @@ const char* RedisReplyTypeToString(RedisReplyType type) { } } +bool RedisReply::SerializeToIOBuf(butil::IOBuf* buf) { Review comment: SerializeTo就行了,以后若需要针对其他buffer类型增加override也很自然。 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356973158 ## File path: src/brpc/policy/redis_protocol.cpp ## @@ -52,62 +58,206 @@ struct InputResponse : public InputMessageBase { } }; -// "Message" = "Response" as we only implement the client for redis. -ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket, - bool /*read_eof*/, const void* /*arg*/) { -if (source->empty()) { -return MakeParseError(PARSE_ERROR_NOT_ENOUGH_DATA); +// This class is as parsing_context in socket. +class RedisConnContext : public Destroyable { +public: +RedisConnContext() +: redis_service(NULL) +, handler_continue(NULL) {} +~RedisConnContext(); +// @Destroyable +void Destroy() override; + +int Init(); + +SocketId socket_id; +RedisService* redis_service; +// If user starts a transaction, handler_continue indicates the +// first handler pointer that triggers the transaction. +RedisCommandHandler* handler_continue; +// The redis command are parsed and pushed into this queue +bthread::ExecutionQueueId queue; + +RedisCommandParser parser; +}; + +int ConsumeTask(RedisConnContext* ctx, std::string* command, butil::IOBuf* sendbuf) { +butil::Arena arena; +RedisReply output(&arena); +if (ctx->handler_continue) { +RedisCommandHandler::Result result = +ctx->handler_continue->Run(command->c_str(), &output); +if (result == RedisCommandHandler::OK) { +ctx->handler_continue = NULL; +} +} else { +std::string comm; +comm.reserve(8); +for (int i = 0; i < (int)command->size() && (*command)[i] != ' '; ++i) { +comm.push_back(std::tolower((*command)[i])); +} +RedisCommandHandler* ch = ctx->redis_service->FindCommandHandler(comm); +if (!ch) { +char buf[64]; +snprintf(buf, sizeof(buf), "ERR unknown command `%s`", comm.c_str()); +output.SetError(buf); +} else { +RedisCommandHandler::Result result = ch->Run(command->c_str(), &output); +if (result == RedisCommandHandler::CONTINUE) { +ctx->handler_continue = ch; +} +} +} +output.SerializeToIOBuf(sendbuf); +return 0; +} + +int Consume(void* ctx, bthread::TaskIterator& iter) { +RedisConnContext* qctx = static_cast(ctx); +if (iter.is_queue_stopped()) { +delete qctx; +return 0; +} +SocketUniquePtr s; +bool has_err = false; +if (Socket::Address(qctx->socket_id, &s) != 0) { +LOG(WARNING) << "Fail to address redis socket"; Review comment: 这个address应该在插入execq时就address好?那样可以在外面用Readdress 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356952177 ## File path: src/brpc/redis.h ## @@ -124,7 +128,7 @@ class RedisRequest : public ::google::protobuf::Message { void Print(std::ostream&) const; protected: -::google::protobuf::Metadata GetMetadata() const override; +::google::protobuf::Metadata GetMetadata() const; Review comment: 去掉warning-as-error 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #972: Redis server protocol
jamesge commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356954267 ## File path: src/brpc/redis_reply.h ## @@ -128,6 +167,8 @@ class RedisReply { } array; uint64_t padding[2]; // For swapping, must cover all bytes. } _data; +butil::Arena* _arena; +bool _has_set; Review comment: 尽量避免这个字段。 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on issue #993: Does IOPortal consider to support to create memory aligned block for direct IO
jamesge commented on issue #993: Does IOPortal consider to support to create memory aligned block for direct IO URL: https://github.com/apache/incubator-brpc/issues/993#issuecomment-564868544 A work-around: allocate and read into the buffer by you own, and manage&share the buffer by IOBuf through [append_user_data](https://github.com/apache/incubator-brpc/blob/master/src/butil/iobuf.h#L250) 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #991: Support protocol parsing order
jamesge commented on a change in pull request #991: Support protocol parsing order URL: https://github.com/apache/incubator-brpc/pull/991#discussion_r356976911 ## File path: src/brpc/global.cpp ## @@ -556,7 +575,7 @@ static void GlobalInitializeOrDieImpl() { NULL, NULL, NULL, (ConnectionType)(CONNECTION_TYPE_SINGLE|CONNECTION_TYPE_SHORT), "rtmp" }; -if (RegisterProtocol(PROTOCOL_RTMP, rtmp_protocol) != 0) { +if (RegisterProtocol(PROTOCOL_RTMP, rtmp_protocol, order_map) != 0) { Review comment: 为什么要传完整的一个map?非常奇怪,也无法让其他模块用好这个功能 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #991: Support protocol parsing order
jamesge commented on a change in pull request #991: Support protocol parsing order URL: https://github.com/apache/incubator-brpc/pull/991#discussion_r356976719 ## File path: src/brpc/input_messenger.cpp ## @@ -86,7 +86,7 @@ ParseResult InputMessenger::CutInputMessage( } if (m->CreatedByConnect() && // baidu_std may fall to streaming_rpc -(ProtocolType)preferred != PROTOCOL_BAIDU_STD) { +strcmp(_handlers[preferred].name, "baidu_std") != 0) { Review comment: 这个效率不行,这里是hotpath 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on a change in pull request #972: Redis server protocol
zyearn commented on a change in pull request #972: Redis server protocol URL: https://github.com/apache/incubator-brpc/pull/972#discussion_r356990098 ## File path: src/brpc/policy/redis_protocol.cpp ## @@ -52,62 +58,206 @@ struct InputResponse : public InputMessageBase { } }; -// "Message" = "Response" as we only implement the client for redis. -ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket, - bool /*read_eof*/, const void* /*arg*/) { -if (source->empty()) { -return MakeParseError(PARSE_ERROR_NOT_ENOUGH_DATA); +// This class is as parsing_context in socket. +class RedisConnContext : public Destroyable { +public: +RedisConnContext() +: redis_service(NULL) +, handler_continue(NULL) {} +~RedisConnContext(); +// @Destroyable +void Destroy() override; + +int Init(); + +SocketId socket_id; +RedisService* redis_service; +// If user starts a transaction, handler_continue indicates the +// first handler pointer that triggers the transaction. +RedisCommandHandler* handler_continue; +// The redis command are parsed and pushed into this queue +bthread::ExecutionQueueId queue; + +RedisCommandParser parser; +}; + +int ConsumeTask(RedisConnContext* ctx, std::string* command, butil::IOBuf* sendbuf) { +butil::Arena arena; +RedisReply output(&arena); +if (ctx->handler_continue) { +RedisCommandHandler::Result result = +ctx->handler_continue->Run(command->c_str(), &output); +if (result == RedisCommandHandler::OK) { +ctx->handler_continue = NULL; +} +} else { +std::string comm; +comm.reserve(8); +for (int i = 0; i < (int)command->size() && (*command)[i] != ' '; ++i) { +comm.push_back(std::tolower((*command)[i])); +} +RedisCommandHandler* ch = ctx->redis_service->FindCommandHandler(comm); +if (!ch) { +char buf[64]; +snprintf(buf, sizeof(buf), "ERR unknown command `%s`", comm.c_str()); +output.SetError(buf); +} else { +RedisCommandHandler::Result result = ch->Run(command->c_str(), &output); +if (result == RedisCommandHandler::CONTINUE) { +ctx->handler_continue = ch; +} +} +} +output.SerializeToIOBuf(sendbuf); +return 0; +} + +int Consume(void* ctx, bthread::TaskIterator& iter) { +RedisConnContext* qctx = static_cast(ctx); +if (iter.is_queue_stopped()) { +delete qctx; +return 0; +} +SocketUniquePtr s; +bool has_err = false; +if (Socket::Address(qctx->socket_id, &s) != 0) { +LOG(WARNING) << "Fail to address redis socket"; +has_err = true; } -// NOTE(gejun): PopPipelinedInfo() is actually more contended than what -// I thought before. The Socket._pipeline_q is a SPSC queue pushed before -// sending and popped when response comes back, being protected by a -// mutex. Previously the mutex is shared with Socket._id_wait_list. When -// 200 bthreads access one redis-server, ~1.5s in total is spent on -// contention in 10-second duration. If the mutex is separated, the time -// drops to ~0.25s. I further replaced PeekPipelinedInfo() with -// GivebackPipelinedInfo() to lock only once(when receiving response) -// in most cases, and the time decreases to ~0.14s. -PipelinedInfo pi; -if (!socket->PopPipelinedInfo(&pi)) { -LOG(WARNING) << "No corresponding PipelinedInfo in socket"; -return MakeParseError(PARSE_ERROR_TRY_OTHERS); -} - -do { -InputResponse* msg = static_cast(socket->parsing_context()); -if (msg == NULL) { -msg = new InputResponse; -socket->reset_parsing_context(msg); +Socket::WriteOptions wopt; +wopt.ignore_eovercrowded = true; +butil::IOBuf sendbuf; +for (; iter; ++iter) { +std::unique_ptr guard(*iter); +if (has_err) { +continue; } +ConsumeTask(qctx, *iter, &sendbuf); +// If there are too many tasks to execute, latency of the front +// responses will be increased by waiting the following tasks to +// be completed. To prevent this, if the current buf size is greater +// than FLAGS_redis_batch_flush_max_size, we just write the current +// buf first. +if ((int)sendbuf.size() >= FLAGS_redis_batch_flush_data_size) { +LOG_IF(WARNING, s->Write(&sendbuf, &wopt) != 0) +<< "Fail to send redis reply"; +} +} +if (!has_err && !sendbuf.empty()) { +LOG_IF(WARNING, s->Write(&sendbuf, &wopt) != 0) +<< "Fail to send redis reply"; +} +return 0; +} + +// == impl of RedisConnContext == -const int consume_count = (pi.with_auth ? 1 : pi.count); +RedisConnContext::~RedisConnContext
[GitHub] [incubator-brpc] ten2ton opened a new issue #994: Link to contributing page is invalid
ten2ton opened a new issue #994: Link to contributing page is invalid URL: https://github.com/apache/incubator-brpc/issues/994 **Describe the bug (描述bug)** In project home site community page, link to contributing page is invalid. **To Reproduce (复现方法)** **Expected behavior (期望行为)** **Versions (各种版本)** OS: Compiler: brpc: protobuf: **Additional context/screenshots (更多上下文/截图)** 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org