Skip to content

Commit

Permalink
SET working, todo remaining tests fix
Browse files Browse the repository at this point in the history
  • Loading branch information
@hamdaankhalidmsft
hamdaankhalidmsft committed Dec 20, 2024
1 parent 7f5170d commit 5c2337d
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 32 deletions.
9 changes: 5 additions & 4 deletions libs/server/Resp/BasicCommands.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -814,11 +814,12 @@ private bool NetworkSET_Conditional<TGarnetApi>(RespCommand cmd, int expiry, ref
SendAndReset();
break;

case (_, true, RespCommand.SET, GarnetStatus.NOTFOUND): // since SET with etag goes down RMW a not found is okay and data is on buffer
// since SET with etag goes down RMW a not found is okay and data is on buffer
case (_, true, RespCommand.SET, GarnetStatus.NOTFOUND):
// if getvalue || etag and Status is OK then the response is always on the buffer, getvalue is never used with conditionals
// extra pattern matching on command below for invariant get value cannot be used with EXXX and EXNX
case (true, _, RespCommand.SET or RespCommand.SETIFMATCH or RespCommand.SETKEEPTTL, GarnetStatus.OK):
case (_, true, _, GarnetStatus.OK):
case (_, true, _, GarnetStatus.OK or GarnetStatus.NOTFOUND):
if (!outputBuffer.IsSpanByte)
SendAndReset(outputBuffer.Memory, outputBuffer.Length);
else
Expand All @@ -834,8 +835,8 @@ private bool NetworkSET_Conditional<TGarnetApi>(RespCommand cmd, int expiry, ref
break;

case (_, _, RespCommand.SETEXNX, GarnetStatus.OK): // For NX semantics an OK indicates a found, which means nothing was set and hence we return NIL
// anything not found that did not come from SETEXNX always returns NIL, also anything that is indicating wrong type or moved will return NIL
case (_, _, _, GarnetStatus.NOTFOUND or GarnetStatus.WRONGTYPE or GarnetStatus.MOVED):
// anything not found that did not come from SETEXNX or WITHETAG always returns NIL, also anything that is indicating wrong type or moved will return NIL
case (_, false, not RespCommand.SETEXNX, GarnetStatus.NOTFOUND or GarnetStatus.WRONGTYPE or GarnetStatus.MOVED):
while (!RespWriteUtils.WriteDirect(CmdStrings.RESP_ERRNOTFOUND, ref dcurr, dend))
SendAndReset();
break;
Expand Down
33 changes: 26 additions & 7 deletions libs/server/Storage/Functions/MainStore/RMWMethods.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ public bool NeedInitialUpdate(ref SpanByte key, ref RawStringInput input, ref Sp
{
case RespCommand.SETIFMATCH:
case RespCommand.SETKEEPTTLXX:
case RespCommand.SETEXXX:
case RespCommand.PERSIST:
case RespCommand.EXPIRE:
case RespCommand.PEXPIRE:
Expand All @@ -30,6 +29,14 @@ public bool NeedInitialUpdate(ref SpanByte key, ref RawStringInput input, ref Sp
case RespCommand.GETDEL:
case RespCommand.GETEX:
return false;
case RespCommand.SETEXXX:
// when called withetag all output needs to be placed on the buffer
if (input.header.CheckWithEtagFlag())
{
// EXX when unsuccesful will write back NIL
CopyDefaultResp(CmdStrings.RESP_ERRNOTFOUND, ref output);
}
return false;
default:
if (input.header.cmd > RespCommandExtensions.LastValidCommand)
{
Expand Down Expand Up @@ -94,12 +101,8 @@ public bool InitialUpdater(ref SpanByte key, ref RawStringInput input, ref SpanB
{
// the increment on initial etag is for satisfying the variant that any key with no etag is the same as a zero'd etag
*(long*)value.ToPointer() = Constants.BaseEtag + 1;
if (cmd == RespCommand.SET)
{
// Copy initial etag to output only for SET + WITHETAG and not SET NX or XX
CopyRespNumber(Constants.BaseEtag + 1, ref output);
}

// Copy initial etag to output only for SET + WITHETAG and not SET NX or XX
CopyRespNumber(Constants.BaseEtag + 1, ref output);
}
break;

Expand Down Expand Up @@ -306,6 +309,14 @@ private bool InPlaceUpdaterWorker(ref SpanByte key, ref RawStringInput input, re
// Copy value to output for the GET part of the command.
CopyRespTo(ref value, ref output, etagIgnoredOffset, etagIgnoredEnd);
}

// when called withetag all output needs to be placed on the buffer
if (input.header.CheckWithEtagFlag())
{
// EXX when unsuccesful will write back NIL
CopyDefaultResp(CmdStrings.RESP_ERRNOTFOUND, ref output);
}

// Nothing is set because being in this block means NX was already violated
return true;
case RespCommand.SETIFMATCH:
Expand Down Expand Up @@ -787,6 +798,14 @@ public bool NeedCopyUpdate(ref SpanByte key, ref RawStringInput input, ref SpanB
// Copy value to output for the GET part of the command.
CopyRespTo(ref oldValue, ref output, etagIgnoredOffset, etagIgnoredEnd);
}

// when called withetag all output needs to be placed on the buffer
if (input.header.CheckWithEtagFlag())
{
// EXX when unsuccesful will write back NIL
CopyDefaultResp(CmdStrings.RESP_ERRNOTFOUND, ref output);
}

// since this block is only hit when this an update, the NX is violated and so we can return early from it without setting the value
return false;
case RespCommand.SETEXXX:
Expand Down
43 changes: 22 additions & 21 deletions test/Garnet.test/RespEtagTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,7 @@ public void SetWithEtagWorksWithMetadata()
var key2 = "key2";
db.Execute("SET", key2, "value2", "WITHETAG");
var res2 = db.Execute("SET", key2, "value3", "WITHETAG", "NX", "EX", 10);
// it 1 if key set, 0 if not set
ClassicAssert.AreEqual(1, (int)res2);
ClassicAssert.IsTrue(res2.IsNull);
db.KeyDelete(key2); // Cleanup

// Scenario: set with etag with expiration NX with non-existent key
Expand Down Expand Up @@ -263,24 +262,24 @@ public void SETWithWITHETAGOnAlreadyExistingSETDataOverridesItButUpdatesEtag()
// update to value to update the etag
RedisResult[] updateRes = (RedisResult[])db.Execute("SETIFMATCH", ["rizz", "fixx", etag.ToString()]);
etag = (long)updateRes[0];
ClassicAssert.AreEqual(1, etag);
ClassicAssert.AreEqual(2, etag);
ClassicAssert.AreEqual("fixx", updateRes[1].ToString());

// inplace update
res = db.Execute("SET", ["rizz", "meow", "WITHETAG"]);
etag = (long)res;
ClassicAssert.AreEqual(2, etag);
ClassicAssert.AreEqual(3, etag);

// update to value to update the etag
updateRes = (RedisResult[])db.Execute("SETIFMATCH", ["rizz", "fooo", etag.ToString()]);
etag = (long)updateRes[0];
ClassicAssert.AreEqual(3, etag);
ClassicAssert.AreEqual(4, etag);
ClassicAssert.AreEqual("fooo", updateRes[1].ToString());

// Copy update
res = db.Execute("SET", ["rizz", "oneofus", "WITHETAG"]);
etag = (long)res;
ClassicAssert.AreEqual(4, etag);
ClassicAssert.AreEqual(5, etag);
}

[Test]
Expand Down Expand Up @@ -522,7 +521,7 @@ public void SetExpiryIncrForEtagSetData()

// This should increase the ETAG internally so we have a check for that here
checkEtag = long.Parse(db.Execute("GETWITHETAG", [strKey])[0].ToString());
ClassicAssert.AreEqual(2, checkEtag);
ClassicAssert.AreEqual(3, checkEtag);

nRetVal = Convert.ToInt64(db.StringGet(strKey));
ClassicAssert.AreEqual(n, nRetVal);
Expand Down Expand Up @@ -567,15 +566,15 @@ public void IncrDecrChangeDigitsWithExpiry()
ClassicAssert.AreEqual(10, nRetVal);

checkEtag = long.Parse(db.Execute("GETWITHETAG", [strKey])[0].ToString());
ClassicAssert.AreEqual(1, checkEtag);
ClassicAssert.AreEqual(2, checkEtag);

n = db.StringDecrement(strKey);
nRetVal = Convert.ToInt64(db.StringGet(strKey));
ClassicAssert.AreEqual(n, nRetVal);
ClassicAssert.AreEqual(9, nRetVal);

checkEtag = long.Parse(db.Execute("GETWITHETAG", [strKey])[0].ToString());
ClassicAssert.AreEqual(2, checkEtag);
ClassicAssert.AreEqual(3, checkEtag);

Thread.Sleep(TimeSpan.FromSeconds(5));

Expand Down Expand Up @@ -630,7 +629,7 @@ public void StringSetOnAnExistingEtagDataUpdatesEtagIfEtagRetain()
ClassicAssert.AreEqual("ciaociao", retVal);

var res = (RedisResult[])db.Execute("GETWITHETAG", strKey);
ClassicAssert.AreEqual(1, (long)res[0]);
ClassicAssert.AreEqual(2, (long)res[0]);

// on subsequent upserts we are still increasing the etag transparently
db.Execute("SET", [strKey, "ciaociaociao", "WITHETAG"]);
Expand All @@ -639,7 +638,7 @@ public void StringSetOnAnExistingEtagDataUpdatesEtagIfEtagRetain()
ClassicAssert.AreEqual("ciaociaociao", retVal);

res = (RedisResult[])db.Execute("GETWITHETAG", strKey);
ClassicAssert.AreEqual(2, (long)res[0]);
ClassicAssert.AreEqual(3, (long)res[0]);
ClassicAssert.AreEqual("ciaociaociao", res[1].ToString());
}

Expand Down Expand Up @@ -1169,12 +1168,13 @@ public void PersistTTLTestForEtagSetData()

db.KeyExpire(key, TimeSpan.FromSeconds(expire));
res = (RedisResult[])db.Execute("GETWITHETAG", [key]);
ClassicAssert.AreEqual(0, long.Parse(res[0].ToString()));
ClassicAssert.AreEqual(1, long.Parse(res[0].ToString()));
ClassicAssert.AreEqual(val, res[1].ToString());

db.KeyPersist(key);
res = (RedisResult[])db.Execute("GETWITHETAG", [key]);
ClassicAssert.AreEqual(0, long.Parse(res[0].ToString()));
// unchanged etag
ClassicAssert.AreEqual(1, long.Parse(res[0].ToString()));
ClassicAssert.AreEqual(val, res[1].ToString());

Thread.Sleep((expire + 1) * 1000);
Expand All @@ -1186,7 +1186,8 @@ public void PersistTTLTestForEtagSetData()
ClassicAssert.IsNull(time);

res = (RedisResult[])db.Execute("GETWITHETAG", [key]);
ClassicAssert.AreEqual(0, long.Parse(res[0].ToString()));
// the tag was persisted along with data from persist despite previous TTL
ClassicAssert.AreEqual(1, long.Parse(res[0].ToString()));
ClassicAssert.AreEqual(val, res[1].ToString());
}

Expand Down Expand Up @@ -1508,7 +1509,7 @@ public void SetRangeTestForEtagSetData()

// should update the etag internally
updatedEtagRes = db.Execute("GETWITHETAG", key);
ClassicAssert.AreEqual(1, long.Parse(updatedEtagRes[0].ToString()));
ClassicAssert.AreEqual(2, long.Parse(updatedEtagRes[0].ToString()));

ClassicAssert.IsTrue(db.KeyDelete(key));

Expand Down Expand Up @@ -1719,8 +1720,8 @@ public void AppendTestForEtagSetData()
ClassicAssert.AreEqual(val, _val.ToString());

etagToCheck = long.Parse(((RedisResult[])db.Execute("GETWITHETAG", [key]))[0].ToString());
// we appended nothing so this remains 0
ClassicAssert.AreEqual(0, etagToCheck);
// we appended nothing so this remains 1
ClassicAssert.AreEqual(1, etagToCheck);

// Test appending to a non-existent key
var nonExistentKey = "nonExistentKey";
Expand All @@ -1739,14 +1740,14 @@ public void AppendTestForEtagSetData()
ClassicAssert.AreEqual(largeVal.Length + val2.Length, len3);

etagToCheck = long.Parse(((RedisResult[])db.Execute("GETWITHETAG", [key]))[0].ToString());
ClassicAssert.AreEqual(1, etagToCheck);
ClassicAssert.AreEqual(2, etagToCheck);

// Test appending to a key with metadata
var keyWithMetadata = "keyWithMetadata";
db.Execute("SET", [keyWithMetadata, val, "WITHETAG"]);
db.KeyExpire(keyWithMetadata, TimeSpan.FromSeconds(10000));
etagToCheck = long.Parse(((RedisResult[])db.Execute("GETWITHETAG", [keyWithMetadata]))[0].ToString());
ClassicAssert.AreEqual(0, etagToCheck);
ClassicAssert.AreEqual(1, etagToCheck);

var len4 = db.StringAppend(keyWithMetadata, val2);
ClassicAssert.AreEqual(val.Length + val2.Length, len4);
Expand All @@ -1758,7 +1759,7 @@ public void AppendTestForEtagSetData()
ClassicAssert.IsTrue(time.Value.TotalSeconds > 0);

etagToCheck = long.Parse(((RedisResult[])db.Execute("GETWITHETAG", [keyWithMetadata]))[0].ToString());
ClassicAssert.AreEqual(1, etagToCheck);
ClassicAssert.AreEqual(2, etagToCheck);
}

[Test]
Expand Down Expand Up @@ -1912,7 +1913,7 @@ public void BitFieldIncrementWithSaturateOverflowOnEtagSetData()
var incrResult = db.Execute("BITFIELD", key, "OVERFLOW", "SAT", "INCRBY", "u8", "0", "10");

etagToCheck = long.Parse(((RedisResult[])db.Execute("GETWITHETAG", [key]))[0].ToString());
ClassicAssert.AreEqual(2, etagToCheck);
ClassicAssert.AreEqual(3, etagToCheck);

// Assert
ClassicAssert.AreEqual(255, (long)incrResult); // Should saturate at the max value of 255 for u8
Expand Down

0 comments on commit 5c2337d

Please sign in to comment.