Failure on AddOrUpdate

[dimitriospafos]

Describe the bug
I've been on 2.0.0-beta-1629 for a while and trying to update to 2.0.0. When I use 2.0.0 with my current setup which is AWS Elasticache with ValKey engine, cluster mode enabled it seems like half of the time AddOrUpdate keeps failing and retrying but it ends up not inserting or updating it.

To Reproduce
Steps to reproduce the behavior:
Call AddOrUpdate method a few times on an AWS cluster mode enabled instance.

Expected behavior
The key to be instered or updated

Screenshots
Attaching the log trace

Additional context

failing_logs.txt

[dimitriospafos]

Looks like the lua script for "Add" is not cluster mode compliant. Basically I see a "redis call" on KEYS[1] as well as on ARGV[7] which might hash to different nodes and cause an issue. In the older beta package the "Add" script only makes "redis call" on KEYS[1] which always hashes and runs on the same node.

ADD SCRIPT
2.0.0:

private static readonly string _scriptAdd = $@"
if redis.call('HSETNX', KEYS[1], '{HashFieldValue}', ARGV[1]) == 1 then
    if ARGV[7] ~= nil then
        if (redis.call('HMSET', ARGV[7], KEYS[1], 'regionKey')) then
        else
            return -2
        end
    end
    local result=redis.call('HMSET', KEYS[1], '{HashFieldType}', ARGV[2], '{HashFieldExpirationMode}', ARGV[3], '{HashFieldExpirationTimeout}', ARGV[4], '{HashFieldCreated}', ARGV[5], '{HashFieldVersion}', 1, '{HashFieldUsesDefaultExp}', ARGV[6])
    if ARGV[3] > '1' and ARGV[4] ~= '0' then
        redis.call('PEXPIRE', KEYS[1], ARGV[4])
    else
        redis.call('PERSIST', KEYS[1])
    end
    return 1
else
    return nil
end";

2.0.0-beta-1629:

 private static readonly string _scriptAdd = "\n
  if redis.call('HSETNX', KEYS[1], 'value', ARGV[1]) == 1 then
     local result=redis.call('HMSET', KEYS[1], 'type', ARGV[2], 'expiration', ARGV[3], 'timeout', ARGV[4], 'created', ARGV[5], 'version', 1, 'defaultExpiration', ARGV[6])
	 if ARGV[3] > '1' and ARGV[4] ~= '0' then
		redis.call('PEXPIRE', KEYS[1], ARGV[4])
	else        
		redis.call('PERSIST', KEYS[1])
	end
		return result
	else
		return nil
	end"; 

[MichaCo]

Thanks for trying to find out what the reason was ;)
And I think you got it.

The second key on ARGV[7] is the region key, would be totally fine if that key is stored elsewhere, the problem here is that I'm using args instead of a key.

While digging through the documentation I remember reading that a long time ago but obviously forgot about the importance of declaring all keys when calling EVAL.
The current implementation does use one of the args as key which probably causes the bad hash issue...

[MichaCo]

Would it be possible for you to test the PR version before I release that? @dimitriospafos
You can find the new nugets here https://www.myget.org/feed/Packages/dnsclient

[dimitriospafos]

Sure, let me give it a try. Thank you for looking into it.

[dimitriospafos]

Seems like this wont do it.
Getting an error now: "StackExchange.Redis.RedisCommandException: 'Multi-key operations must involve a single slot; keys can use 'hash tags' to help this, i.e. '{/users/12345}/account' and '{/users/12345}/contacts' will always be in the same slot'"

I think we cant pass multiple keys here "return _connection.Database.ScriptEvaluate(script.Hash, new[] { redisKey, regionKeyPrep }, values, flags);"

[MichaCo]

I think we cant pass multiple keys here "return _connection.Database.ScriptEvaluate(script.Hash, new[] { redisKey, regionKeyPrep }, values, flags);"

This will very likely cause the same error because its going to do the exact same thing ;)

I think the only good solution is to use hashtags in Redis.
According to documentation, that's the way to enforce that certain sets of keys lang on the same hash in a cluster.
It works kind of like a prefix, anything starting with that key + surrounded by "{" and "}" ends up on the same hash.

I can change the region logic to use "{<regionKey}:key" instead of what I used before.

That might actually be a better implementation in general because I used ":" so far as separator. And when users used ":" in their key or region key as a separator themselves, I switch to base64 encoding those keys.
I think I could remove that workaround after switching to hashtags.

That being said, that's a major change which would be incompatible with any currently stored region keys. Which means I cannot just make that a minor bugfix unfortunately.
This will also need a lot of testing

[MichaCo]

Hey @dimitriospafos
I pushed the latest changes as a pre-release package to nuget.org,
Could you give that a try?
I have to do more testing before I release that and also wanted to complete other things first, too.

But, yea, let me know please ;)

thanks

[dimitriospafos]

Hi, yes, let me give it a try.

The only concern about using hashtags is that, some use-cases have a lot of keys under one region key which might end up hashing most of the keys to the same node. But for my use-cases it should be fine.

[MichaCo]

Right, but you can just not use regions if you have those kind of issues :)