Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce commandlog heavytraffic to record big response packet. #336

Closed
wants to merge 9 commits into from
Closed

Introduce commandlog heavytraffic to record big response packet. #336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6548769
Implement fat log.
CharlesChen888 Apr 19, 2024
e4eb38b
Rename file and correct typo.
CharlesChen888 Apr 19, 2024
b273214
Rename. Fix details.
CharlesChen888 May 29, 2024
a3ee175
Run clang format.
CharlesChen888 May 29, 2024
e1ab7d9
Fatlog count input big packet as well.
CharlesChen888 May 30, 2024
e928f27
Minor fix in comments.
CharlesChen888 Jul 1, 2024
a791ac0
Run clang format.
CharlesChen888 Jul 1, 2024
204dfba
Record whole query packer instead of just arguments.
CharlesChen888 Jul 2, 2024
55f1a52
Merge branch 'unstable' into add-fat-log.
CharlesChen888 Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 111 additions & 1 deletion src/commands.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6737,6 +6737,115 @@ struct COMMAND_ARG FAILOVER_Args[] = {
{MAKE_ARG("milliseconds",ARG_TYPE_INTEGER,-1,"TIMEOUT",NULL,NULL,CMD_ARG_OPTIONAL,0,NULL)},
};

/********** FATLOG GET ********************/

#ifndef SKIP_CMD_HISTORY_TABLE
/* FATLOG GET history */
#define FATLOG_GET_History NULL
#endif

#ifndef SKIP_CMD_TIPS_TABLE
/* FATLOG GET tips */
const char *FATLOG_GET_Tips[] = {
"request_policy:all_nodes",
"nondeterministic_output",
};
#endif

#ifndef SKIP_CMD_KEY_SPECS_TABLE
/* FATLOG GET key specs */
#define FATLOG_GET_Keyspecs NULL
#endif

/* FATLOG GET argument table */
struct COMMAND_ARG FATLOG_GET_Args[] = {
{MAKE_ARG("count",ARG_TYPE_INTEGER,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,0,NULL)},
};

/********** FATLOG HELP ********************/

#ifndef SKIP_CMD_HISTORY_TABLE
/* FATLOG HELP history */
#define FATLOG_HELP_History NULL
#endif

#ifndef SKIP_CMD_TIPS_TABLE
/* FATLOG HELP tips */
#define FATLOG_HELP_Tips NULL
#endif

#ifndef SKIP_CMD_KEY_SPECS_TABLE
/* FATLOG HELP key specs */
#define FATLOG_HELP_Keyspecs NULL
#endif

/********** FATLOG LEN ********************/

#ifndef SKIP_CMD_HISTORY_TABLE
/* FATLOG LEN history */
#define FATLOG_LEN_History NULL
#endif

#ifndef SKIP_CMD_TIPS_TABLE
/* FATLOG LEN tips */
const char *FATLOG_LEN_Tips[] = {
"request_policy:all_nodes",
"response_policy:agg_sum",
"nondeterministic_output",
};
#endif

#ifndef SKIP_CMD_KEY_SPECS_TABLE
/* FATLOG LEN key specs */
#define FATLOG_LEN_Keyspecs NULL
#endif

/********** FATLOG RESET ********************/

#ifndef SKIP_CMD_HISTORY_TABLE
/* FATLOG RESET history */
#define FATLOG_RESET_History NULL
#endif

#ifndef SKIP_CMD_TIPS_TABLE
/* FATLOG RESET tips */
const char *FATLOG_RESET_Tips[] = {
"request_policy:all_nodes",
"response_policy:all_succeeded",
};
#endif

#ifndef SKIP_CMD_KEY_SPECS_TABLE
/* FATLOG RESET key specs */
#define FATLOG_RESET_Keyspecs NULL
#endif

/* FATLOG command table */
struct COMMAND_STRUCT FATLOG_Subcommands[] = {
{MAKE_CMD("get","Returns the fat log's entries.","O(N) where N is the number of entries returned","8.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,FATLOG_GET_History,0,FATLOG_GET_Tips,2,slowlogCommand,-2,CMD_ADMIN|CMD_LOADING|CMD_STALE,0,FATLOG_GET_Keyspecs,0,NULL,1),.args=FATLOG_GET_Args},
{MAKE_CMD("help","Returns helpful text about the different subcommands","O(1)","8.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,FATLOG_HELP_History,0,FATLOG_HELP_Tips,0,slowlogCommand,2,CMD_LOADING|CMD_STALE,0,FATLOG_HELP_Keyspecs,0,NULL,0)},
{MAKE_CMD("len","Returns the number of entries in the fat log.","O(1)","8.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,FATLOG_LEN_History,0,FATLOG_LEN_Tips,3,slowlogCommand,2,CMD_ADMIN|CMD_LOADING|CMD_STALE,0,FATLOG_LEN_Keyspecs,0,NULL,0)},
{MAKE_CMD("reset","Clears all entries from the fat log.","O(N) where N is the number of entries in the fatlog","8.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,FATLOG_RESET_History,0,FATLOG_RESET_Tips,2,slowlogCommand,2,CMD_ADMIN|CMD_LOADING|CMD_STALE,0,FATLOG_RESET_Keyspecs,0,NULL,0)},
{0}
};

/********** FATLOG ********************/

#ifndef SKIP_CMD_HISTORY_TABLE
/* FATLOG history */
#define FATLOG_History NULL
#endif

#ifndef SKIP_CMD_TIPS_TABLE
/* FATLOG tips */
#define FATLOG_Tips NULL
#endif

#ifndef SKIP_CMD_KEY_SPECS_TABLE
/* FATLOG key specs */
#define FATLOG_Keyspecs NULL
#endif

/********** FLUSHALL ********************/

#ifndef SKIP_CMD_HISTORY_TABLE
Expand Down Expand Up @@ -7692,7 +7801,7 @@ const char *SLOWLOG_RESET_Tips[] = {
/* SLOWLOG command table */
struct COMMAND_STRUCT SLOWLOG_Subcommands[] = {
{MAKE_CMD("get","Returns the slow log's entries.","O(N) where N is the number of entries returned","2.2.12",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,SLOWLOG_GET_History,1,SLOWLOG_GET_Tips,2,slowlogCommand,-2,CMD_ADMIN|CMD_LOADING|CMD_STALE,0,SLOWLOG_GET_Keyspecs,0,NULL,1),.args=SLOWLOG_GET_Args},
{MAKE_CMD("help","Show helpful text about the different subcommands","O(1)","6.2.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,SLOWLOG_HELP_History,0,SLOWLOG_HELP_Tips,0,slowlogCommand,2,CMD_LOADING|CMD_STALE,0,SLOWLOG_HELP_Keyspecs,0,NULL,0)},
{MAKE_CMD("help","Returns helpful text about the different subcommands","O(1)","6.2.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,SLOWLOG_HELP_History,0,SLOWLOG_HELP_Tips,0,slowlogCommand,2,CMD_LOADING|CMD_STALE,0,SLOWLOG_HELP_Keyspecs,0,NULL,0)},
{MAKE_CMD("len","Returns the number of entries in the slow log.","O(1)","2.2.12",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,SLOWLOG_LEN_History,0,SLOWLOG_LEN_Tips,3,slowlogCommand,2,CMD_ADMIN|CMD_LOADING|CMD_STALE,0,SLOWLOG_LEN_Keyspecs,0,NULL,0)},
{MAKE_CMD("reset","Clears all entries from the slow log.","O(N) where N is the number of entries in the slowlog","2.2.12",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,SLOWLOG_RESET_History,0,SLOWLOG_RESET_Tips,2,slowlogCommand,2,CMD_ADMIN|CMD_LOADING|CMD_STALE,0,SLOWLOG_RESET_Keyspecs,0,NULL,0)},
{0}
Expand Down Expand Up @@ -10905,6 +11014,7 @@ struct COMMAND_STRUCT serverCommandTable[] = {
{MAKE_CMD("dbsize","Returns the number of keys in the database.","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,DBSIZE_History,0,DBSIZE_Tips,2,dbsizeCommand,1,CMD_READONLY|CMD_FAST,ACL_CATEGORY_KEYSPACE,DBSIZE_Keyspecs,0,NULL,0)},
{MAKE_CMD("debug","A container for debugging commands.","Depends on subcommand.","1.0.0",CMD_DOC_SYSCMD,NULL,NULL,"server",COMMAND_GROUP_SERVER,DEBUG_History,0,DEBUG_Tips,0,debugCommand,-2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_PROTECTED,0,DEBUG_Keyspecs,0,NULL,0)},
{MAKE_CMD("failover","Starts a coordinated failover from a server to one of its replicas.","O(1)","6.2.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,FAILOVER_History,0,FAILOVER_Tips,0,failoverCommand,-1,CMD_ADMIN|CMD_NOSCRIPT|CMD_STALE,0,FAILOVER_Keyspecs,0,NULL,3),.args=FAILOVER_Args},
{MAKE_CMD("fatlog","A container for fat log commands.","Depends on subcommand.","8.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,FATLOG_History,0,FATLOG_Tips,0,NULL,-2,0,0,FATLOG_Keyspecs,0,NULL,0),.subcommands=FATLOG_Subcommands},
{MAKE_CMD("flushall","Removes all keys from all databases.","O(N) where N is the total number of keys in all databases","1.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,FLUSHALL_History,2,FLUSHALL_Tips,2,flushallCommand,-1,CMD_WRITE,ACL_CATEGORY_KEYSPACE|ACL_CATEGORY_DANGEROUS,FLUSHALL_Keyspecs,0,NULL,1),.args=FLUSHALL_Args},
{MAKE_CMD("flushdb","Remove all keys from the current database.","O(N) where N is the number of keys in the selected database","1.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,FLUSHDB_History,2,FLUSHDB_Tips,2,flushdbCommand,-1,CMD_WRITE,ACL_CATEGORY_KEYSPACE|ACL_CATEGORY_DANGEROUS,FLUSHDB_Keyspecs,0,NULL,1),.args=FLUSHDB_Args},
{MAKE_CMD("info","Returns information and statistics about the server.","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,INFO_History,1,INFO_Tips,3,infoCommand,-1,CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_DANGEROUS,INFO_Keyspecs,0,NULL,1),.args=INFO_Args},
Expand Down
69 changes: 69 additions & 0 deletions src/commands/fatlog-get.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I understand the rational behind a new command, a fat command is logically different from a slow command, I would also like us to more seriously consider just adding one new field to the slowlog, the output/input bytes, and then having both get logged to the same place. It's not a perfect fit, but then users can just look in "one" place for bad behavior. It also allows the two logs to perhaps explain each other, since a large output can cause a slow command. We could extend SLOWLOG GET [count] [TYPE FAT|SLOW] to only show entries of the specific type.

I'm thinking about it more from a simpler way to explain it to end users.

Copy link
Member Author

@CharlesChen888 CharlesChen888 May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does make things easier than adding a new series of commands, if we can ignore that changes in format of command reply may cause backwards compatibility problem.
But a big packet in query or response does not necessarily mean it will be slow. I'm not sure if we should do this.

"GET": {
"summary": "Returns the fat log's entries.",
"complexity": "O(N) where N is the number of entries returned",
"group": "server",
"since": "8.0.0",
"arity": -2,
"container": "FATLOG",
"function": "slowlogCommand",
"history": [],
"command_flags": [
"ADMIN",
"LOADING",
"STALE"
],
"command_tips": [
"REQUEST_POLICY:ALL_NODES",
"NONDETERMINISTIC_OUTPUT"
],
"reply_schema": {
"type": "array",
"description": "Entries from the slow log in chronological order.",
"uniqueItems": true,
"items": {
"type": "array",
"minItems": 6,
"maxItems": 6,
"items": [
{
"type": "integer",
"description": "Fat log entry ID."
},
{
"type": "integer",
"description": "The unix timestamp at which the logged command was processed.",
"minimum": 0
},
{
"type": "integer",
"description": "The size of the response to the query in bytes.",
"minimum": 0
},
{
"type": "array",
"description": "The arguments of the command.",
"items": {
"type": "string"
}
},
{
"type": "string",
"description": "Client IP address and port."
},
{
"type": "string",
"description": "Client name if set via the CLIENT SETNAME command."
}
]
}
},
"arguments": [
{
"name": "count",
"type": "integer",
"optional": true
}
]
}
}
22 changes: 22 additions & 0 deletions src/commands/fatlog-help.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"HELP": {
"summary": "Returns helpful text about the different subcommands",
"complexity": "O(1)",
"group": "server",
"since": "8.0.0",
"arity": 2,
"container": "FATLOG",
"function": "slowlogCommand",
"command_flags": [
"LOADING",
"STALE"
],
"reply_schema": {
"type": "array",
"description": "Helpful text about subcommands.",
"items": {
"type": "string"
}
}
}
}
26 changes: 26 additions & 0 deletions src/commands/fatlog-len.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"LEN": {
"summary": "Returns the number of entries in the fat log.",
"complexity": "O(1)",
"group": "server",
"since": "8.0.0",
"arity": 2,
"container": "FATLOG",
"function": "slowlogCommand",
"command_flags": [
"ADMIN",
"LOADING",
"STALE"
],
"command_tips": [
"REQUEST_POLICY:ALL_NODES",
"RESPONSE_POLICY:AGG_SUM",
"NONDETERMINISTIC_OUTPUT"
],
"reply_schema": {
"type": "integer",
"description": "Number of entries in the fat log.",
"minimum": 0
}
}
}
23 changes: 23 additions & 0 deletions src/commands/fatlog-reset.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"RESET": {
"summary": "Clears all entries from the fat log.",
"complexity": "O(N) where N is the number of entries in the fatlog",
"group": "server",
"since": "8.0.0",
"arity": 2,
"container": "FATLOG",
"function": "slowlogCommand",
"command_flags": [
"ADMIN",
"LOADING",
"STALE"
],
"command_tips": [
"REQUEST_POLICY:ALL_NODES",
"RESPONSE_POLICY:ALL_SUCCEEDED"
],
"reply_schema": {
"const": "OK"
}
}
}
9 changes: 9 additions & 0 deletions src/commands/fatlog.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"FATLOG": {
"summary": "A container for fat log commands.",
"complexity": "Depends on subcommand.",
"group": "server",
"since": "8.0.0",
"arity": -2
}
}
2 changes: 1 addition & 1 deletion src/commands/slowlog-help.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"HELP": {
"summary": "Show helpful text about the different subcommands",
"summary": "Returns helpful text about the different subcommands",
"complexity": "O(1)",
"group": "server",
"since": "6.2.0",
Expand Down
2 changes: 2 additions & 0 deletions src/config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3173,6 +3173,7 @@ standardConfig static_configs[] = {
createULongConfig("active-defrag-max-scan-fields", NULL, MODIFIABLE_CONFIG, 1, LONG_MAX, server.active_defrag_max_scan_fields, 1000, INTEGER_CONFIG, NULL, NULL), /* Default: keys with more than 1000 fields will be processed separately */
createULongConfig("slowlog-max-len", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.slowlog_max_len, 128, INTEGER_CONFIG, NULL, NULL),
createULongConfig("acllog-max-len", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.acllog_max_len, 128, INTEGER_CONFIG, NULL, NULL),
createULongConfig("fatlog-max-len", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.fatlog_max_len, 128, INTEGER_CONFIG, NULL, NULL),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add these 2 new config parameter in valkey.conf and describe details, pls reference the Slow Log section.


/* Long Long configs */
createLongLongConfig("busy-reply-threshold", "lua-time-limit", MODIFIABLE_CONFIG, 0, LONG_MAX, server.busy_reply_threshold, 5000, INTEGER_CONFIG, NULL, NULL), /* milliseconds */
Expand All @@ -3183,6 +3184,7 @@ standardConfig static_configs[] = {
createLongLongConfig("proto-max-bulk-len", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, 1024 * 1024, LONG_MAX, server.proto_max_bulk_len, 512ll * 1024 * 1024, MEMORY_CONFIG, NULL, NULL), /* Bulk request max size */
createLongLongConfig("stream-node-max-entries", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.stream_node_max_entries, 100, INTEGER_CONFIG, NULL, NULL),
createLongLongConfig("repl-backlog-size", NULL, MODIFIABLE_CONFIG, 1, LLONG_MAX, server.repl_backlog_size, 1024 * 1024, MEMORY_CONFIG, NULL, updateReplBacklogSize), /* Default: 1mb */
createLongLongConfig("fatlog-log-bigger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.fatlog_log_bigger_than, 1024 * 1024, MEMORY_CONFIG, NULL, NULL),

/* Unsigned Long Long configs */
createULongLongConfig("maxmemory", NULL, MODIFIABLE_CONFIG, 0, ULLONG_MAX, server.maxmemory, 0, MEMORY_CONFIG, NULL, updateMaxmemory),
Expand Down
17 changes: 17 additions & 0 deletions src/networking.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ client *createClient(connection *conn) {
c->qb_pos = 0;
c->querybuf = NULL;
c->querybuf_peak = 0;
c->cmd_query_length = 0;
c->reqtype = 0;
c->argc = 0;
c->argv = NULL;
Expand Down Expand Up @@ -186,6 +187,7 @@ client *createClient(connection *conn) {
c->reply = listCreate();
c->deferred_reply_errors = NULL;
c->reply_bytes = 0;
c->cmd_reply_length = 0;
c->obuf_soft_limit_reached_time = 0;
listSetFreeMethod(c->reply, freeClientReplyValue);
listSetDupMethod(c->reply, dupClientReplyValue);
Expand Down Expand Up @@ -442,6 +444,9 @@ void _addReplyToBufferOrList(client *c, const char *s, size_t len) {
* buffer offset (see function comment) */
reqresSaveClientReplyOffset(c);

/* Record reply length. */
c->cmd_reply_length += len;

/* If we're processing a push message into the current client (i.e. executing PUBLISH
* to a channel which we are subscribed to, then we wanna postpone that message to be added
* after the command's reply (specifically important during multi-exec). the exception is
Expand Down Expand Up @@ -794,6 +799,9 @@ void setDeferredReply(client *c, void *node, const char *s, size_t length) {
if (node == NULL) return;
serverAssert(!listNodeValue(ln));

/* Record reply length. */
c->cmd_reply_length += length;

/* Normally we fill this dummy NULL node, added by addReplyDeferredLen(),
* with a new buffer structure containing the protocol needed to specify
* the length of the array following. However sometimes there might be room
Expand Down Expand Up @@ -2081,6 +2089,7 @@ void resetClient(client *c) {
serverCommandProc *prevcmd = c->cmd ? c->cmd->proc : NULL;

freeClientArgv(c);
c->cmd_query_length = 0;
c->cur_script = NULL;
c->reqtype = 0;
c->multibulklen = 0;
Expand Down Expand Up @@ -2253,6 +2262,9 @@ int processInlineBuffer(client *c) {
/* Move querybuffer position to the next query in the buffer. */
c->qb_pos += querylen + linefeed_chars;

/* Record query length for fatlog. */
c->cmd_query_length = querylen;

/* Setup argv array on client structure */
if (argc) {
if (c->argv) zfree(c->argv);
Expand Down Expand Up @@ -2321,6 +2333,8 @@ int processMultibulkBuffer(client *c) {
int ok;
long long ll;

size_t qb_pos_start = c->qb_pos;

if (c->multibulklen == 0) {
/* The client should have been reset */
serverAssertWithInfo(c, NULL, c->argc == 0);
Expand Down Expand Up @@ -2465,6 +2479,9 @@ int processMultibulkBuffer(client *c) {
}
}

/* Record query length for fatlog. */
c->cmd_query_length += c->qb_pos - qb_pos_start;
Comment on lines +2482 to +2483
Copy link
Contributor

@kyle-yh-kim kyle-yh-kim Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing this commit. The c->qb_pos tracking is definitely a feasible option.
I left some edge-case considerations on the other thread, and will leave the link here for reference; #720 (comment).

We've internally considered using c->qb_pos as well. While tracking c->qb_pos also works, it comes with a few edge cases, namely;

  • c->qb_pos will not start from 0 for some pipeline cases. This means, we must also track the starting position of the qb_pos, call it qb_pos_start. I see that this case is already covered by your commit, which is good.

  • Optimization 1. Large object creation. For the case where a non-primary client's buffer contains just the bulk element, the SDS ownership is simply moved from c->querybuf to robj, rather than copying the entire SDS. In such case, c->qb_pos do not step forward, even though the bytes have actually been read. We must manually account for this, otherwise this ingress bytes is forever lost.

  • Optimization 2. Early c->qb_pos reset. c->qb_pos could be prematurely reset to 0, such that its processing could benefit from the above optimization (creating large object from the 0th index of c->querybuf). In conjunction with the first edge case (where we start from a non-zero qb_pos_start), simply incrementing c->qb_pos - qb_pos_start can yield a negative ingress bytes result.

When comparing both implementations side-by-side, it became clear that c->qb_pos including all its edge case handlings - was harder to comprehend when compared to the c->argv_len_sum variant. Not to mention, c->qb_pos accounting will need to change upon any newly introduced query processing optimizations, and/or changes to the existing optimizations mentioned above.

Ultimately, I find c->argv_len_sum to be a stronger contract. Either subroutines (processInlineBuffer() or processMultibulkBuffer()) hold a guarantee of fully populated c->argc/argv/argv_len_sum before command execution. Any existing or future c->qb_pos optimizations aren't concerned, as long as c->argc/argv/argv_len_sum are correctly filled. I also find the implementation more intuitive and easier to maintain.

But no strong opinions on this, it's good that we are having this discussion, and I'm fine to swing both ways. Curious to hear the core-team's thoughts as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During the latest core meeting (July 15th 2024), we concluded to move forward with the currently upheld c->argv_len_sum approach. I will now resolve this thread.

@CharlesChen888
Given this decision, I would first wait for the merger of per-slot network-bytes-in PR, which introduces c->net_input_bytes_curr_cmd based on c->argv_len_sum calculation. After its merger, your PR may refer to c->net_input_bytes_curr_cmd as a heuristic for assessing fatlogs. With that said, let me know if you need any help from my end. If there's a better way to collaborate, I'm open for it.


/* We're done when c->multibulk == 0 */
if (c->multibulklen == 0) return C_OK;

Expand Down
Loading
Loading