feat(string): support IFEQ/IFNE/IFDEQ/IFDNE in SET command#3475
feat(string): support IFEQ/IFNE/IFDEQ/IFDNE in SET command#3475kirito632 wants to merge 1 commit intoapache:unstablefrom
Conversation
jihuayu
left a comment
jihuayu
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
The SET command is critical, as 99% of our users rely on it.
We must ensure that its test cases are both comprehensive and accurate.
| }) | ||
| } | ||
|
|
||
| func TestSetConditional(t *testing.T) { |
There was a problem hiding this comment.
The new SET conditions fail to inherit the txn-context-enabled matrix. Existing string tests (lines 40, 49) are executed in both modes.
| } | ||
| }) | ||
|
|
||
| // ── Property tests (using testing/quick via subtests) ─────────────────── |
There was a problem hiding this comment.
These aren't property tests; where did you use testing/quick?
Please ensure you've actually read through your code and understand it yourself.
There was a problem hiding this comment.
Apologies for the earlier issues in test scaffolding and comments — I’m fixing them now.
While moving tests into testString, I noticed that the configs parameter is currently ignored:
srv := util.StartServer(t, map[string]string{})So tests are not actually running with txn-context enabled.
Would you prefer me to fix this in this PR, or keep this PR focused and address it separately?
Thanks for your patience!
There was a problem hiding this comment.
Before pushing here, I ran the full CI matrix on my personal fork and noticed two new environment-level failures. I did some triage and confirmed they are upstream environment/toolchain issues, completely unrelated to the SET command changes:
ArchLinux Container (jemalloc build failure): The rolling release updated to GCC 16.1.1, which removed the internal std::__throw_bad_alloc() symbol. This breaks jemalloc 5.3.1 during compilation. Jemalloc upstream has fixed this in master, but since 5.4.0 isn't out, it fails the build here.
Ubuntu ARM Clang (Server Startup Failure): The TestReplicationGroupSyncConfig fails exclusively on the ARM64 Clang runner with a connection refused error. The Kvrocks server process seems to exit unexpectedly during startup without generating an "Ooops!" crash log. Since the exact same code passes perfectly on ARM GCC and all x86 runners, this appears to be an ARM64 Clang-specific compiler or environment compatibility issue.
Please let me know your thoughts on how to proceed!
There was a problem hiding this comment.
We known and fix will be included into this PR #3478 or separated. Thanks about your investigation
There was a problem hiding this comment.
I have updated the PR to address the two issues you pointed out (matrix inheritance and comments).
I also passed the actual configs parameter into util.StartServer.
I have pre-verified the full CI matrix on my personal fork and it is strictly green.
|
Add IFEQ/IFNE/IFDEQ/IFDNE conditionals to SET command, extending the existing NX/XX/GET subcommand support. Move conditional SET tests into testString to inherit txn-context-enabled matrix, and add non-hex 16-char digest boundary test for IFDEQ/IFDNE.
3 participants
What
Reintroduce conditional options IFEQ / IFNE / IFDEQ / IFDNE for the SET command.
Context
This is a clean, reworked version of the previously closed #3452.
It addresses all previous feedback, reverts the accidental deletion of
CommandDelEX, and is fully rebased onto the latestunstablebranch.All CI checks have been successfully run and verified on my personal fork before submission.
Behavior
WRONGTYPEfor non-string keysSET key value NX GETnow correctly returnsWRONGTYPEfor non-string keys (matching Redis 8.x behavior)Testing
AI-assisted Contribution Disclosure
AI was used for code pattern suggestions, test scaffolding, and debugging assistance.
Core logic, bug fixes, validation, and final implementation were written and verified manually.
@jihuayu PTAL, thanks!