chore: use BackedArguments for memcache_parser #6176
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the memcache parser to use BackedArguments for storing command keys, replacing the previous custom memory management approach with a more consistent, reusable pattern already used elsewhere in the codebase. This simplifies MemcacheParser::Command by removing the key and keys_ext members, as well as the internal blob storage member, in favor of inheriting from BackedArguments.
Key changes:
MemcacheParser::Commandnow inherits fromBackedArgumentsfor key storage- Added iterator support to
BackedArgumentsto enable range-based operations - Simplified
MCPipelineMessageby storing value asstd::stringinstead of managing separate backing storage
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/backed_args.h | Added iterator support and empty() method to BackedArguments to support memcache command iteration |
| src/facade/memcache_parser.h | Refactored Command struct to inherit from BackedArguments; replaced key and keys_ext with BackedArguments storage; added key() accessor method |
| src/facade/memcache_parser.cc | Updated parsing logic to use tmp_args_ temporary storage and MaterializeArgs helper; introduced critical bug in base64 decoding with dangling reference |
| src/facade/memcache_parser_test.cc | Updated tests to use cmd_.key() accessor and new ToArgs() helper; fixed test logic to check all keys starting from index 0 |
| src/facade/dragonfly_connection.h | Simplified MCPipelineMessage to store value as std::string instead of managing custom backing storage |
| src/facade/dragonfly_connection.cc | Simplified MCPipelineMessage constructor; updated memory tracking to use BackedArguments::HeapMemory() |
| src/server/main_service.cc | Updated DispatchMC to work with BackedArguments iterator interface and new key() accessor |
| src/server/test_utils.cc | Updated test utilities to use BackedArguments::Assign() for constructing memcache commands |
|
|
||
| class MCParserTest : public testing::Test { | ||
| protected: | ||
| MemcacheParser::Result Parse(string_view input) { |
There was a problem hiding this comment.
Despite many changes, the tests here are left intact. most changes are around Parse called being replaced with this auxillary function.
| size_t index_; | ||
| size_t offset_ = 0; |
There was a problem hiding this comment.
As I understand, offset we can get from index, so I don't see any point in storing it here
There was a problem hiding this comment.
you need O(N) to compute offset from lens
offset = Sum{len_[k] | k <= index }
There was a problem hiding this comment.
just for curiosity if instead of len we will store offset directly and calculate len like next - prev. It should solve the complexity
There was a problem hiding this comment.
interesting idea, let me think about it.
There was a problem hiding this comment.
for 3 elements with len 5 4 3 you will have the next offset array [5, 9, 12] to get len return id > 0 ? offset[id] - offset[id-1] : offset[0];
romange
force-pushed
the
MC1
branch
2 times, most recently
from
a45c532 to
a1c1010
Compare
romange
force-pushed
the
MC1
branch
3 times, most recently
from
739f3ff to
0f55bc7
Compare
Signed-off-by: Roman Gershman <roman@dragonflydb.io>
|
augment review |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| cmd->clear(); | ||
|
|
||
| if (cmd->type <= CAS) { // Store command | ||
| if (tokens_view.size() < 4 || tokens[0].size() > 250) { // key length limit |
There was a problem hiding this comment.
This key length check uses tokens[0], which is the command name, not the key; consider checking tokens_view[0] so the 250-char key limit is actually enforced.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
will double check in the next PR.
|
|
||
| if (res->type >= MP::DELETE) { // write commands | ||
| if (!res->keys_ext.empty() && res->keys_ext.back() == "noreply") { | ||
| if (args->size() > 1 && args->back() == "noreply") { |
There was a problem hiding this comment.
should this comparison be case insensitive? args->back() == "noreply"
There was a problem hiding this comment.
- it's not a regression
- based on https://github.com/memcached/memcached/blob/master/doc/protocol.txt#L209 it should be lowcase.
Change BackedArguments to keep prefix sums of all lengths of its items Based on the idea from #6176 (comment) Signed-off-by: Roman Gershman <roman@dragonflydb.io>
Change BackedArguments to keep prefix sums of all lengths of its items Based on the idea from #6176 (comment) Signed-off-by: Roman Gershman <roman@dragonflydb.io>
Change BackedArguments to keep prefix sums of all lengths of its items Based on the idea from #6176 (comment) Signed-off-by: Roman Gershman <roman@dragonflydb.io>
Following #6171 and #6169 we implement Memcache::Command using BackedArguments.