Add Memcached meta-command protocol support (memcache_meta)#363
Open
sam-obeid wants to merge 2 commits intoredis:masterfrom
Open
Add Memcached meta-command protocol support (memcache_meta)#363sam-obeid wants to merge 2 commits intoredis:masterfrom
sam-obeid wants to merge 2 commits intoredis:masterfrom
Conversation
Adds a new value 'memcache_meta' to the -P/--protocol flag, enabling
benchmarking of the Memcached meta protocol (memcached 1.6+).
Wire format
-----------
Every command is wire-terminated by 'mn\r\n' so every response is
uniformly terminated by 'MN'. This eliminates write-time/parse-time
mode-attribution races under pipelined / mixed-command workloads:
SET: ms <k> <l> T<e>\r\n<value>\r\n mn\r\n -> HD\r\n MN\r\n
GET: mg <k> v\r\n mn\r\n -> VA <n>\r\n<data>\r\n MN\r\n
(or EN MN on miss)
multi-GET: pipelined 'mg ... v\r\n' + 'mn\r\n' -> ...VA/EN..., MN
The wire cost is +4 request +4 response bytes per command; at typical
value sizes (>= 64B) this is < 1% of bandwidth.
Implementation
--------------
The new memcache_meta_protocol class inherits from memcache_text_protocol
to reuse the buffer plumbing and the seven assert(0) stubs for operations
not supported by either Memcached path (auth, select_db, cluster_slots,
wait, arbitrary_command). Only the four wire-touching methods are
overridden: write_command_set / write_command_get / write_command_multi_get
/ parse_response.
Performance touches:
- mg / multi-get write paths use raw evbuffer_add (no vsnprintf in the
per-key hot path), matching the existing memcache_text multi-get pattern.
- VA <size> parsing uses strtoul (not sscanf) on the GET hot path.
- Response state machine has a single termination rule (read until MN),
removing all conditional branches on external state in the hot loop.
Validation against memcached 1.6.41 (4 clients x 2 threads x 10K ops):
meta SETs 135K ops/sec (text: 131K)
meta GETs 130K ops/sec (text: 133K, ~3.5x less response bandwidth)
meta multi-get(4) 256K keys/sec
mixed --pipeline=4 246K ops/sec
Tests
-----
13 unit tests in tests/test_memcache_meta_protocol.py exercise the
parser via an in-process mock TCP server that speaks the meta protocol,
so the suite runs without requiring memcached or Redis. Coverage:
Happy path: single SET, single GET (hit / miss), GET against
pre-loaded data, multi-get with mn terminator,
SET -> GET roundtrip
Status codes: NS / EX / NF responses do not error
Async / partial: fragmented VA delivery (header and value across
two TCP sends -> rs_read_value 'need more data' path)
Error path: unknown server response classified as parse error
Pipelining: --pipeline=4 SET, mixed --pipeline=2 SET +
multi-get (regression for write-time/parse-time race)
Multi-get: deterministic VA + EN mix within a single pipeline
Tests are also discoverable by RLTest via top-level test_meta_*
adapters so they ride along with ./tests/run_tests.sh.
Documentation
-------------
Updated --help text, error message for invalid -P, manpage
(memtier_benchmark.1), README.md feature bullet, and bash completion.
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
Addresses two issues found in code review (PR redis#363): 1. get_protocol_name() had no case for PROTOCOL_MEMCACHE_META, causing --show-config and the JSON output file to report 'protocol = none' instead of 'memcache_meta'. Add the missing branch. 2. The --authenticate validation only rejected PROTOCOL_MEMCACHE_TEXT. Passing --authenticate with --protocol=memcache_meta hit the inherited memcache_text_protocol::authenticate() assert(0), producing a confusing crash report instead of the same clean usage error the text path emits. Extend the guard to reject memcache_meta as well. Verified manually: - '-P memcache_meta --show-config' now prints 'protocol = memcache_meta' - The JSON output reports 'protocol: memcache_meta' - '-P memcache_meta --authenticate=foo' now exits with the clean error 'authenticate can only be used with redis or memcache_binary' instead of crashing. - Existing text / binary / redis auth paths unchanged. - All 13 meta unit tests still pass.
Author
|
cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7d2eddd. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a new value 'memcache_meta' to the -P/--protocol flag, enabling benchmarking of the Memcached meta protocol (memcached 1.6+).
Wire format
Every command is wire-terminated by 'mn\r\n' so every response is uniformly terminated by 'MN'. This eliminates write-time/parse-time mode-attribution races under pipelined / mixed-command workloads:
The wire cost is +4 request +4 response bytes per command; at typical value sizes (>= 64B) this is < 1% of bandwidth.
Implementation
The new memcache_meta_protocol class inherits from memcache_text_protocol to reuse the buffer plumbing and the seven assert(0) stubs for operations not supported by either Memcached path (auth, select_db, cluster_slots, wait, arbitrary_command). Only the four wire-touching methods are overridden: write_command_set / write_command_get / write_command_multi_get / parse_response.
Performance touches:
Validation against memcached 1.6.41 (4 clients x 2 threads x 10K ops):
meta SETs 135K ops/sec (text: 131K)
meta GETs 130K ops/sec (text: 133K, ~3.5x less response bandwidth)
meta multi-get(4) 256K keys/sec
mixed --pipeline=4 246K ops/sec
Tests
13 unit tests in tests/test_memcache_meta_protocol.py exercise the parser via an in-process mock TCP server that speaks the meta protocol, so the suite runs without requiring memcached or Redis. Coverage:
Happy path: single SET, single GET (hit / miss), GET against
pre-loaded data, multi-get with mn terminator,
SET -> GET roundtrip
Status codes: NS / EX / NF responses do not error
Async / partial: fragmented VA delivery (header and value across
two TCP sends -> rs_read_value 'need more data' path)
Error path: unknown server response classified as parse error
Pipelining: --pipeline=4 SET, mixed --pipeline=2 SET +
multi-get (regression for write-time/parse-time race)
Multi-get: deterministic VA + EN mix within a single pipeline
Tests are also discoverable by RLTest via top-level test_meta_* adapters so they ride along with ./tests/run_tests.sh.
Documentation
Updated --help text, error message for invalid -P, manpage (memtier_benchmark.1), README.md feature bullet, and bash completion.
Note
Medium Risk
Adds a new Memcached meta protocol implementation with custom request/response parsing and pipelining behavior, which can impact benchmark correctness and connection stability. Risk is moderated by extensive new protocol-specific integration tests and mainly additive changes.
Overview
Adds support for benchmarking Memcached’s modern meta-command ASCII protocol via a new
-P/--protocol=memcache_metaoption, including a dedicated protocol implementation (memcache_meta_protocol) and factory/CLI wiring.Implements meta wire formats for SET/GET/multi-GET and a new response parser that treats
MNas a universal terminator to avoid mixed-command pipelining mode races, and updates validation to disallow--authenticatefor this protocol.Updates user-facing docs/help (README, manpage,
--helptext, bash completion) and adds a comprehensive Python test suite with an in-process mock meta server to verify exact wire format, parsing behavior (hits/misses, fragmentation), error handling, and pipelined mixed workloads.Reviewed by Cursor Bugbot for commit 7d2eddd. Bugbot is set up for automated code reviews on this repo. Configure here.