-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: Implement MSETEX command #6334
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.
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
base: main
Are you sure you want to change the base?
Conversation
🤖 Augment PR SummarySummary: Introduces the Redis-compatible Changes:
Technical Notes: Execution is coordinated via a single-hop transaction and uses explicit journaling to replicate the resulting per-key updates (including TTL via 🤖 Was this summary useful? React with 👍 or 👎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements the MSETEX command, which atomically sets multiple string keys with a shared expiration time, extending MSET functionality with expiration options similar to SET. The command syntax is MSETEX numkeys key value [key value ...] [EX seconds|PX milliseconds|EXAT timestamp|PXAT timestamp|KEEPTTL], returning 1 on success or 0 on failure. A notable deviation from the original issue is that NX/XX options are intentionally not supported due to atomicity constraints in Dragonfly's multi-threaded architecture.
Key changes:
- Extended transaction key determination logic to properly handle VARIADIC_KEYS with INTERLEAVED_KEYS patterns
- Added ParseExpireTime helper function for consistent expiration time validation across commands
- Implemented OpMSetEx and CmdMSetEx for the core MSETEX functionality with explicit journaling
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/server/transaction.cc | Extended DetermineKeys to support VARIADIC_KEYS with INTERLEAVED_KEYS by adjusting end calculation and adding bounds validation; updated DCHECK to use NumArgs() for correct assertion with interleaved keys |
| src/server/string_family.cc | Added ParseExpireTime helper, OpMSetEx operation handler with explicit journaling, CmdMSetEx command implementation with expiration option parsing, and registered MSETEX command with VARIADIC_KEYS and INTERLEAVED_KEYS flags |
| src/server/string_family_test.cc | Added comprehensive MSetEx test covering basic usage, expiration options (EX, PX, KEEPTTL), error cases, and verification that NX/XX options return unsupported option errors |
| TEST_F(StringFamilyTest, MSetEx) { | ||
| // Basic usage - sets two keys with expiration | ||
| EXPECT_EQ(1, CheckedInt({"msetex", "2", "key1", "val1", "key2", "val2", "EX", "100"})); | ||
| EXPECT_EQ(Run({"get", "key1"}), "val1"); | ||
| EXPECT_EQ(Run({"get", "key2"}), "val2"); | ||
| EXPECT_THAT(Run({"ttl", "key1"}), IntArg(100)); | ||
| EXPECT_THAT(Run({"ttl", "key2"}), IntArg(100)); | ||
|
|
||
| // Basic usage without expiration | ||
| EXPECT_EQ(1, CheckedInt({"msetex", "1", "key3", "val3"})); | ||
| EXPECT_EQ(Run({"get", "key3"}), "val3"); | ||
| EXPECT_THAT(Run({"ttl", "key3"}), IntArg(-1)); | ||
|
|
||
| // PX option (milliseconds) | ||
| EXPECT_EQ(1, CheckedInt({"msetex", "1", "key4", "val4", "PX", "5000"})); | ||
| EXPECT_EQ(Run({"get", "key4"}), "val4"); | ||
| auto ttl = Run({"pttl", "key4"}); | ||
| EXPECT_GT(get<int64_t>(ttl.u), 4000); | ||
|
|
||
| // KEEPTTL option | ||
| Run({"set", "ttl_key", "original", "EX", "200"}); | ||
| EXPECT_EQ(1, CheckedInt({"msetex", "1", "ttl_key", "new_value", "KEEPTTL"})); | ||
| EXPECT_EQ(Run({"get", "ttl_key"}), "new_value"); | ||
| auto ttl_val = Run({"ttl", "ttl_key"}); | ||
| EXPECT_GT(get<int64_t>(ttl_val.u), 0); | ||
| EXPECT_LE(get<int64_t>(ttl_val.u), 200); | ||
|
|
||
| // Error cases | ||
| EXPECT_THAT(Run({"msetex", "abc", "k", "v"}), ErrArg("not an integer")); // invalid numkeys | ||
| EXPECT_THAT(Run({"msetex", "0", "k", "v"}), ErrArg("not an integer")); // numkeys <= 0 | ||
| EXPECT_THAT(Run({"msetex", "2", "k", "v"}), ErrArg("syntax")); // not enough key-value pairs | ||
| EXPECT_THAT(Run({"msetex", "1", "k", "v", "EX", "10", "PX", "1000"}), | ||
| ErrArg("syntax")); // multiple expiration options | ||
| EXPECT_THAT(Run({"msetex", "1", "k", "v", "EX", "0"}), | ||
| ErrArg("invalid expire time")); // zero expiration | ||
| EXPECT_THAT(Run({"msetex", "1", "k", "v", "EX", "-1"}), | ||
| ErrArg("invalid expire time")); // negative expiration | ||
|
|
||
| // NX/XX options are unsupported due to atomicity constraints in multi-threaded execution | ||
| EXPECT_THAT(Run({"msetex", "1", "k", "v", "NX"}), ErrArg("unsupported option")); | ||
| EXPECT_THAT(Run({"msetex", "1", "k", "v", "XX"}), ErrArg("unsupported option")); | ||
| EXPECT_THAT(Run({"msetex", "2", "k1", "v1", "k2", "v2", "NX", "EX", "100"}), | ||
| ErrArg("unsupported option")); | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test coverage is missing for EXAT and PXAT expiration options. While EX, PX, and KEEPTTL are tested, absolute timestamp expiration options (EXAT and PXAT) should also be tested to ensure they work correctly with MSETEX. Consider adding tests similar to those that exist for the SET command with EXAT/PXAT options.
| auto expire_ms = ParseExpireTime(int_val, is_ms, is_absolute); | ||
| if (!expire_ms) { | ||
| return cmnd_cntx->SendError(InvalidExpireTime("msetex")); | ||
| } | ||
|
|
||
| sparams.flags |= SetCmd::SET_EXPIRE_AFTER_MS; | ||
| sparams.expire_after_ms = *expire_ms; | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing handling for expiration times in the past. When using EXAT or PXAT with a timestamp that has already passed, the SET command (lines 1062-1076) has special logic to delete existing keys and return success. However, MSETEX doesn't handle this case - it will successfully set keys with negative relative expiration times. The keys should either be deleted immediately (like SET does) or the operation should fail with an appropriate error message.
Adds MSETEX command that atomically sets multiple string keys with a shared expiration time, extending MSET with expiration options similar to SET. Syntax: MSETEX numkeys key value [key value ...] [EX seconds|PX milliseconds|EXAT timestamp|PXAT timestamp|KEEPTTL] Returns: 1 if all keys were set 0 if operation failed Note: NX/XX options are intentionally not supported as ensuring atomicity for these conditional operations is much harder in Dragonfly's multi-threaded architecture. These options will return "unsupported option" error. Implementation: - OpMSetEx: Handles per-shard operations with expiration options - CmdMSetEx: Parses command arguments and coordinates transaction - DetermineKeys: Extended to handle VARIADIC_KEYS with INTERLEAVED_KEYS - Added bounds validation for interleaved key-value arguments Testing: - Comprehensive unit tests covering all expiration options (EX, PX, EXAT, PXAT, KEEPTTL) - Tests for NX/XX rejection with unsupported option error - Error handling tests for invalid arguments Fixes #6329 Signed-off-by: Roman Gershman <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| // The returned value is suitable for SetCmd::SetParams::expire_after_ms. | ||
| optional<uint64_t> ParseExpireTime(int64_t int_val, bool is_ms, bool is_absolute) { | ||
| DbSlice::ExpireParams expiry{ | ||
| .value = int_val, | ||
| .unit = is_ms ? TimeUnit::MSEC : TimeUnit::SEC, | ||
| .absolute = is_absolute, | ||
| }; | ||
|
|
||
| int64_t now_ms = GetCurrentTimeMs(); | ||
| auto [rel_ms, abs_ms] = expiry.Calculate(now_ms, false); | ||
| // Reject if absolute time is negative or if expiration is already in the past | ||
| if (abs_ms < 0 || rel_ms < 0) { | ||
| return nullopt; | ||
| } | ||
|
|
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ParseExpireTime function has a bug when handling absolute timestamps (EXAT/PXAT) that are in the past. When an absolute timestamp is provided that is positive but less than the current time, the calculated relative time (rel_msec) will be negative. This negative int64_t value is then implicitly converted to uint64_t when returned, causing an integer overflow that results in a very large positive expiration time instead of rejecting the invalid timestamp or treating it as expired. The function should check if the calculated relative time is negative or zero when absolute timestamps are used and return nullopt in such cases.
| TEST_F(StringFamilyTest, MSetEx) { | ||
| // Basic usage - sets two keys with expiration | ||
| EXPECT_EQ(1, CheckedInt({"msetex", "2", "key1", "val1", "key2", "val2", "EX", "100"})); | ||
| EXPECT_EQ(Run({"get", "key1"}), "val1"); | ||
| EXPECT_EQ(Run({"get", "key2"}), "val2"); | ||
| EXPECT_THAT(Run({"ttl", "key1"}), IntArg(100)); | ||
| EXPECT_THAT(Run({"ttl", "key2"}), IntArg(100)); | ||
|
|
||
| // Basic usage without expiration | ||
| EXPECT_EQ(1, CheckedInt({"msetex", "1", "key3", "val3"})); | ||
| EXPECT_EQ(Run({"get", "key3"}), "val3"); | ||
| EXPECT_THAT(Run({"ttl", "key3"}), IntArg(-1)); | ||
|
|
||
| // PX option (milliseconds) | ||
| EXPECT_EQ(1, CheckedInt({"msetex", "1", "key4", "val4", "PX", "5000"})); | ||
| EXPECT_EQ(Run({"get", "key4"}), "val4"); | ||
| auto ttl = Run({"pttl", "key4"}); | ||
| EXPECT_GT(get<int64_t>(ttl.u), 4000); | ||
|
|
||
| // EXAT option (absolute timestamp in seconds) | ||
| int64_t future_ts = time(nullptr) + 100; | ||
| EXPECT_EQ(1, CheckedInt({"msetex", "1", "exat_key", "val", "EXAT", to_string(future_ts)})); | ||
| EXPECT_EQ(Run({"get", "exat_key"}), "val"); | ||
| auto exat_ttl = Run({"ttl", "exat_key"}); | ||
| EXPECT_GT(get<int64_t>(exat_ttl.u), 90); | ||
| EXPECT_LE(get<int64_t>(exat_ttl.u), 100); | ||
|
|
||
| // PXAT option (absolute timestamp in milliseconds) | ||
| int64_t future_ts_ms = (time(nullptr) + 100) * 1000; | ||
| EXPECT_EQ(1, CheckedInt({"msetex", "1", "pxat_key", "val", "PXAT", to_string(future_ts_ms)})); | ||
| EXPECT_EQ(Run({"get", "pxat_key"}), "val"); | ||
| auto pxat_ttl = Run({"pttl", "pxat_key"}); | ||
| EXPECT_GT(get<int64_t>(pxat_ttl.u), 90000); | ||
| EXPECT_LE(get<int64_t>(pxat_ttl.u), 100000); | ||
|
|
||
| // KEEPTTL option | ||
| Run({"set", "ttl_key", "original", "EX", "200"}); | ||
| EXPECT_EQ(1, CheckedInt({"msetex", "1", "ttl_key", "new_value", "KEEPTTL"})); | ||
| EXPECT_EQ(Run({"get", "ttl_key"}), "new_value"); | ||
| auto ttl_val = Run({"ttl", "ttl_key"}); | ||
| EXPECT_GT(get<int64_t>(ttl_val.u), 0); | ||
| EXPECT_LE(get<int64_t>(ttl_val.u), 200); | ||
|
|
||
| // Error cases |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test suite is missing coverage for EXAT and PXAT options which are mentioned in the command syntax and implementation. Tests should be added to verify that absolute timestamps work correctly, including edge cases like timestamps in the future, timestamps around the current time, and invalid timestamps (e.g., timestamps in the past).
Adds MSETEX command that atomically sets multiple string keys with a shared expiration time, extending MSET with expiration options similar to SET.
Syntax:
MSETEX numkeys key value [key value ...] [EX seconds|PX milliseconds|EXAT timestamp|PXAT timestamp|KEEPTTL]
Returns:
1 if all keys were set
0 if operation failed
Note: NX/XX options are intentionally not supported as ensuring atomicity for these conditional operations is much harder in Dragonfly's multi-threaded architecture. These options will return "unsupported option" error.
Implementation:
Testing:
Fixes #6329