wwbmmm commented on code in PR #3665:
URL: https://github.com/apache/brpc/pull/3665#discussion_r4430896708
##########
src/brpc/policy/redis_protocol.cpp:
##########
@@ -177,7 +177,7 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket*
socket,
wopt.ignore_eovercrowded = true;
LOG_IF(WARNING, socket->Write(&sendbuf, &wopt) != 0)
<< "Fail to send redis reply";
- if(ctx->parser.ParsedArgsSize() == 0) {
+ if (ctx->parser.ParsedArgsSize() == 0 && !ctx->transaction_handler) {
Review Comment:
[replied by brpc-oncall robot] Retention here is unconditional while a
transaction handler exists: not only the queued command arguments but also
every `RedisReply output(&ctx->arena)` built during the transaction now stays
allocated until the transaction ends. Handlers that already copy the arguments
(which is what the in-tree MultiTransactionHandler and RedisServiceImpl
examples do) get no benefit and see their per-connection memory grow with all
accumulated transaction data, and a client controls how long a transaction
stays open, so this is now an unbounded, client-driven growth per connection.
Please at least extend the comment above this check to state why the arena must
be retained and who pays for it, and consider whether a cap on retained
transaction data (or an opt-in for handlers that need the views) is needed to
keep the memory behavior bounded.
##########
src/brpc/redis.h:
##########
@@ -255,6 +255,8 @@ class RedisCommandHandler {
// just like what redis-server does.
// `args' is the array of request command. For example, "set somekey
somevalue"
// corresponds to args[0]=="set", args[1]=="somekey" and
args[2]=="somevalue".
+ // The argument strings remain valid until the current transaction
completes.
Review Comment:
[replied by brpc-oncall robot] This comment sits in the generic `Run()`
documentation, but the guarantee only holds for commands dispatched to the
handler created by NewTransactionHandler() (plus the command whose Run()
returned REDIS_CMD_CONTINUE). For a normal handler the arguments are only valid
during Run(). Please scope the sentence explicitly, and make the end point
precise: the storage is released once the read that completes the transaction
finishes parsing, so the views must not be kept after the Run() call that
returns REDIS_CMD_HANDLED (e.g. say "until Run() of the transaction handler
that completes the transaction returns").
--
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]
