Set Command with IFEQ Support #1324

sarthakaggarwal97 · 2024-11-19T19:18:12Z

This PR allows the Valkey users to perform conditional updates where the SET command is completed if the given comparison-value matches the key’s current value.

Behavior with this PR

SET <key> <value> IFEQ <comparison-value>

If the values match, the SET completes as expected. If they do not match, the command returns a (nil).

Behavior with Additional Flags

SET <key> <value> IFEQ <comparison-value> GET returns the existing value if present, (nil) if not and gives the error if there is a Type Mismatch. Conditional set operation is performed if the given comparison value matches the existing value.
SET <key> <value> IFEQ <comparison-value> XX returns nil if the key doesn't exist, otherwise conditionally updates the key if the given comparison value matches the existing value.
SET <key> <value> IFEQ <comparison-value> NX returns nil whether or not the key exists. Does not conditionally update the key if NX flag is present

Addresses: #1215

hpatro

Would be nice to think through the behavior with other flags/parameters of SET command https://valkey.io/commands/set/

NX -- Only set the key if it does not already exist - should we error in this case if CAS is provided?
XX -- Only set the key if it already exists - This becomes redundant to use I believe with CAS.
GET -- Return the old string stored at key, or nil if key did not exist. An error is returned and SET aborted if the value stored at key is not a string. - I think we need to support this parameter in some form. The scenario which comes to my mind is when CAS fails and a user doesn't need to send a GET command again to find out the value stored in the engine. However, we need to think about how to differentiate between success scenario vs failure scenario.

Also, we need to document them.

tests/unit/type/string.tcl

src/t_string.c

codecov · 2024-11-19T22:01:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.76%. Comparing base (4986310) to head (3dc3407).

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1324      +/-   ##
============================================
+ Coverage     70.68%   70.76%   +0.07%     
============================================
  Files           115      115              
  Lines         63177    63198      +21     
============================================
+ Hits          44657    44722      +65     
+ Misses        18520    18476      -44

Files with missing lines	Coverage Δ
src/commands.def	`100.00% <ø> (ø)`
src/t_string.c	`96.88% <100.00%> (+0.13%)`	⬆️

... and 18 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

hpatro · 2024-11-20T04:48:23Z

@sarthakaggarwal97 Please avoid force pushing. force push removes the reviewer's history in Github and one needs to look at the entire change again.

sarthakaggarwal97 · 2024-11-20T04:51:03Z

noted @hpatro, will avoid it.

hpatro

Thanks @sarthakaggarwal97 for the PR! Looks pretty good. Could you also document the behavior in the top comment. Will be easier for others to review and we can finalize it.

hpatro · 2024-11-20T05:05:23Z

tests/unit/type/string.tcl

+        assert_equal {OK} [r set foo "new_value" ifeq "initial_value"]
+        assert_equal "new_value" [r get foo]
+
+        assert_equal {} [r set foo "should_not_set" ifeq "wrong_value"]


@valkey-io/core-team On failure of compare/set instead of nil value we should return an error with old value in it. Otherwise, a client would need to perform another GET operation.

return an error with old value seems odd to me. i think in normal cases, a client is unlikely get an error in set IFEQ, they hold the old value in somewhere, and if the old value is unvalid, this mean client should abort the set, this usually means the client should abort the entire business logic. In this case, client should GET the new value as needed and usually they don't really need the new value, they juse want the result of whether the SET succeeded or not.

@enjoy-binbin I understand if the value is updated from a DB to Valkey as a cache, the client won't benefit much from the value present.

However, if I think Valkey being used as a datastore and value is updated by multiple clients based on the value stored (let say increment old value by 1). for this case, they would need to know the previous value.

Let's hear other devs opinion on this and resolve this.

I like the idea of returning value if the IFEQ fails, not as an error, just as a string. That way you can also check to see if someone else did the same work you did (and silently succeed) or retry you work again.

src/t_string.c

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

src/t_string.c

tests/unit/type/string.tcl

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

hpatro

LGTM overall. The behavior currently is:

if value matches, set the new value.
If existing value is of different type, return error WRONGTYPE Operation against a key holding the wrong kind of value
if existing value is a mismatch, return nil. (want us to finalize on this).

src/commands/set.json

enjoy-binbin · 2024-11-25T02:04:33Z

src/t_string.c

+
+    /* Handle the IFEQ conditional check */
+    if (flags & OBJ_SET_IFEQ && found) {
+        if (!(flags & OBJ_SET_GET) && checkType(c, existing_value, OBJ_STRING)) {


can this happen? i see in the above, we already do this getGenericCommand thing, this will handle all these error path i guess?

if (flags & OBJ_SET_GET) { if (getGenericCommand(c) == C_ERR) return; }

supporting GET and IFEQ look odd to me. GET will get the old value, isn't the value passed in by IFEQ is the same old value?

thanks @enjoy-binbin for the review.

can this happen?

We currently just want to support Check and Set if the existing value is string. The check for OBJ_SET_GET flag prevents us to add additional line of response, when get is supposed to already add response before.

supporting GET and IFEQ look odd to me

I feel it's okay to support GET with IFEQ, in case the user gives an incorrect existing value, and GET can help retrieve it. It would also solve the discussion around if IFEQ should return the value incase the input doesn't matches the existing value (adding GET flag can always do that now).

@madolson @hpatro what do you guys think on this?

src/t_string.c

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

zuiderkwast · 2024-11-27T01:39:54Z

Let's make IFEQ mutually exclusive with NX and XX. It doesn't make sense to combine them so let's make it a syntax error now. (If we don't, we can't fix it in the future without a breaking change.)

SET key value [ NX | XX | IFEQ comparison-value ] [ other args... ]

In the JSON file, putting them in the same "oneOf" block, this is also used for the website and man pages where the syntax is rendered.

Regarding the combining IFEQ with the GET parameter will be difficult to use, but not impossible with the behaviour you described. If SET replied with the 'comparison-value', it means the SET has succeeded. I think it's logical, even if it will probably be very rarely used.

zuiderkwast

This looks pretty good. Just don't want to allow IFEQ combined with NX and XX.

zuiderkwast · 2024-11-27T01:42:03Z

src/commands/set.json

@@ -23,6 +23,10 @@
            [
                "7.0.0",
                "Allowed the `NX` and `GET` options to be used together."
+            ],
+            [
+                "8.2.0",


Our next version is 8.1.0.

Suggested change

"8.2.0",

"8.1.0",

zuiderkwast · 2024-11-27T01:42:14Z

src/commands/set.json

+                "name": "comparison-value",
+                "type": "string",
+                "token": "IFEQ",
+                "since": "8.2.0",


Suggested change

"since": "8.2.0",

"since": "8.1.0",

and move this block to within the "oneOf" for NX | XX below. The "since": "2.6.12" can be moved from the oneOf container to the NX and XX args individually.

I also like this approach. Doesn't make a lot of sense to combine IFEQ with NX | XX.

zuiderkwast · 2024-11-27T01:54:28Z

src/t_string.c

 /* SET key value [NX] [XX] [KEEPTTL] [GET] [EX <seconds>] [PX <milliseconds>]
- *     [EXAT <seconds-timestamp>][PXAT <milliseconds-timestamp>] */
+ *     [EXAT <seconds-timestamp>][PXAT <milliseconds-timestamp>] [IFEQ <comparison-value>] */


zuiderkwast · 2024-11-27T01:58:42Z

src/t_string.c

    int unit = UNIT_SECONDS;
    int flags = OBJ_NO_FLAGS;

-    if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, COMMAND_GET) != C_OK) {
+    if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, &comparison, COMMAND_GET) != C_OK) {


IFEQ is not allowed with COMMAND_GET, so I think we can just pass NULL for the comparison argument. It will not be used. Makes sense?

Suggested change

if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, &comparison, COMMAND_GET) != C_OK) {

if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, NULL, COMMAND_GET) != C_OK) {

zuiderkwast · 2024-11-27T01:59:30Z

tests/assets/test_cli_hint_suite.txt

-"SET key value KEEPTTL " "[NX|XX] [GET]"
-"SET key value XX " "[GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
+"SET key value " "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
+"SET key value EX" "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"


sarthakaggarwal97 force-pushed the cas-command branch from c2dbcba to 14c5fd9 Compare November 19, 2024 19:33

sarthakaggarwal97 marked this pull request as ready for review November 19, 2024 19:47

sarthakaggarwal97 force-pushed the cas-command branch 2 times, most recently from 6c216c1 to ed26e4b Compare November 19, 2024 21:45

hpatro reviewed Nov 19, 2024

View reviewed changes

tests/unit/type/string.tcl Show resolved Hide resolved

src/t_string.c Outdated Show resolved Hide resolved

hpatro added the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Nov 19, 2024

set with ifeq support

4a36392

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

sarthakaggarwal97 force-pushed the cas-command branch 2 times, most recently from 1840d02 to 0652f67 Compare November 20, 2024 03:52

addressing comments and adding tests

b7ed294

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

sarthakaggarwal97 force-pushed the cas-command branch from 0652f67 to b7ed294 Compare November 20, 2024 04:25

hpatro reviewed Nov 20, 2024

View reviewed changes

sarthakaggarwal97 added 5 commits November 19, 2024 21:59

fixes

8b013ed

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

addressing comments

fa789e6

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

addressing tests

07f0fd5

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

clang check

f796577

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

remove dedup lookup

f5ec47c

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

sarthakaggarwal97 mentioned this pull request Nov 20, 2024

Documentation for SET IFEQ valkey-io/valkey-doc#189

Open

hpatro reviewed Nov 20, 2024

View reviewed changes

src/t_string.c Outdated Show resolved Hide resolved

src/t_string.c Outdated Show resolved Hide resolved

src/t_string.c Outdated Show resolved Hide resolved

tests/unit/type/string.tcl Outdated Show resolved Hide resolved

addressing comments

3dc3407

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

sarthakaggarwal97 self-assigned this Nov 20, 2024

hpatro approved these changes Nov 22, 2024

View reviewed changes

hpatro requested a review from madolson November 22, 2024 18:40

enjoy-binbin reviewed Nov 25, 2024

View reviewed changes

addressing comments

62b8832

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

zuiderkwast reviewed Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set Command with IFEQ Support #1324

Set Command with IFEQ Support #1324

sarthakaggarwal97 commented Nov 19, 2024 •

edited

Loading

hpatro left a comment •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading

hpatro commented Nov 20, 2024

sarthakaggarwal97 commented Nov 20, 2024

hpatro left a comment •

edited

Loading

hpatro Nov 20, 2024

enjoy-binbin Nov 21, 2024 •

edited

Loading

hpatro Nov 22, 2024

madolson Nov 25, 2024

hpatro left a comment

enjoy-binbin Nov 25, 2024

sarthakaggarwal97 Nov 25, 2024

zuiderkwast commented Nov 27, 2024 •

edited

Loading

zuiderkwast left a comment

zuiderkwast Nov 27, 2024

zuiderkwast Nov 27, 2024

hpatro Nov 27, 2024

zuiderkwast Nov 27, 2024

zuiderkwast Nov 27, 2024

zuiderkwast Nov 27, 2024

	if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, &comparison, COMMAND_GET) != C_OK) {
	if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, NULL, COMMAND_GET) != C_OK) {

	"SET key value EX" "[IFEQ comparison-value] [NX\|XX] [GET] [EX seconds\|PX milliseconds\|EXAT unix-time-seconds\|PXAT unix-time-milliseconds\|KEEPTTL]"
	"SET key value EX" "[NX\|XX\|IFEQ comparison-value]] [GET] [EX seconds\|PX milliseconds\|EXAT unix-time-seconds\|PXAT unix-time-milliseconds\|KEEPTTL]"

Set Command with IFEQ Support #1324

Are you sure you want to change the base?

Set Command with IFEQ Support #1324

Conversation

sarthakaggarwal97 commented Nov 19, 2024 • edited Loading

Behavior with this PR

Behavior with Additional Flags

hpatro left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 19, 2024 • edited Loading

Codecov Report

hpatro commented Nov 20, 2024

sarthakaggarwal97 commented Nov 20, 2024

hpatro left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enjoy-binbin Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuiderkwast commented Nov 27, 2024 • edited Loading

zuiderkwast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarthakaggarwal97 commented Nov 19, 2024 •

edited

Loading

hpatro left a comment •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading

hpatro left a comment •

edited

Loading

enjoy-binbin Nov 21, 2024 •

edited

Loading

zuiderkwast commented Nov 27, 2024 •

edited

Loading