Allow LoadedLuaScript fail-over in case the Redis instance is restarted

[martintmk]

Motivation

We want to take advantage of sending just hashed LoadedLuaScript to a server, but still, be able automatically failover and reconstruct the hash in case the Redis instance restarts.

The goal was not to change the public API surface.

Adresses #1968

Unit tests are still missing, first I want to check with you guys whether this is the right approach.

[mgravell]

I've had a look at this, and: I think this PR is doing a great job of illustrating the problem, but I'm not sure it is the best direction for the solution. From what I can see, I think we can fix everything here much more simply and consistently, simply by changing LoadedLuaScript to pass ExecutableScript instead of Hash when calling db.ScriptEvaluate[Async](...) (around L268/L285); without any additional changes, I'm pretty sure we would run the lookup against the expected cache automatically, and I'm fine with paying a dictionary trygetvalue for simpler code.

(as a side note: if we did that, we should probably mark the the exposed byte[] Hash as hidden via the usual attributes, and update the remarks; actually, this array is a mild risk currently - it is mutable and prone to abuse)

There is a pre-existing inconsistency between the sync and async versions of ScriptEvaluate[Async](string, ...), and honestly: I think we should just eat the async/await here, and make the code consistent - i.e. reinstate the try/catch (and detect missing script) from the sync version into the async version.

Thoughts?

[martintmk]

Thanks Mark, I am all for simplicity :). If your approach is still able to internally reconstruct and reuse the hash, then it's a better, simpler approach.

I will give this a shot after the weekend.

[martintmk]

@mgravell , I did take a look at the implementation and I think the approach you proposed should work as we will be hitting this code-path (the hashed value is sent):

https://github.com/StackExchange/StackExchange.Redis/blob/d6e05f656a7194a041fd369da779b922ab5c01f0/src/StackExchange.Redis/RedisDatabase.cs#L4804

The only remaining problem is the withKeyPrefix parameter, which is not currently accepted by the IDatabase API: https://github.com/StackExchange/StackExchange.Redis/blob/d6e05f656a7194a041fd369da779b922ab5c01f0/src/StackExchange.Redis/LuaScript.cs#L265

Are we OK with expanding the public API or should I stick with the ParameterAndKeyPrefixHolder trick?

Edit: Also, is there any point of having the LoadedLuaScript as the LoadedLuaScript.Hash is not even used anymore after latest changes...

[mgravell]

Re key prefix; the existing implementation also didn't use any privileged APIs - it just expanded them as part of ExtractParameters - so IMO we should just keep doing that exactly the same

Re LoadedLuaScript at all; we can't remove it because of API signature breaking; it does still at least usually offer some benefit re pre-load of the script; not a massive benefit, perhaps, but: ... I'm not sure it is worth doing much in either direction here, i.e. I'm not convinced we should mark it [Obsolete] either.

[martintmk]

Re key prefix; the existing implementation also didn't use any privileged APIs - it just expanded them as part of ExtractParameters - so IMO we should just keep doing that exactly the same

Yeah, silly me didn't notice that we can just reuse the existing helper. Fixed.

Re LoadedLuaScript at all; we can't remove it because of API signature breaking; it does still at least usually offer some benefit re pre-load of the script; not a massive benefit, perhaps, but: ... I'm not sure it is worth doing much in either direction here, i.e. I'm not convinced we should mark it [Obsolete] either.

Fine by me, are we OK with not using the SHA hash at all then in this code path?

I noticed this piece:

ResultProcessor.ScriptLoadProcessor.IsSHA1(script) ? RedisCommand.EVALSHA : RedisCommand.EVAL

Is it OK if we run ResultProcessor.ScriptLoadProcessor.IsSHA1 on every execution now? (it's simple regex)

[martintmk]

@mgravell After checking this with the team we are still kinda worried about the extra CPU cycles consumed by ResultProcessor.ScriptLoadProcessor.IsSHA1(script).

Do you think we can still re-use the hash in LoadedLuaScript and do an automatic failover?

The problem is how to propagate the information about the missing script from the evaluation call using hash:

Two options comes to mind:

• Introduce RedisResult ScriptEvaluate(LoadedLuaScript script, RedisKey[]? keys = null, RedisValue[]? values = null, CommandFlags flags = CommandFlags.None) - complains about breaking API, since there is another overload that takes LoadedLuaScript
• Propagate the information in the RedisServerException somehow (introduce some internal status field)

WDYT?

[mgravell]

I'd much rather not make the API change here - meaning: the new API added that passes LoadedLuaScript down to avoid the regex; instead, I've tested locally, and if we tweak IsSHA1 trivially: we can just say "yup, that's fine"; I did a test locally of how expensive this regex is:

1000000 tests: 81ms 1000000 tests: 53ms 1000000 tests: 54ms 1000000 tests: 52ms 1000000 tests: 55ms 1000000 tests: 45ms 1000000 tests: 43ms 1000000 tests: 43ms 1000000 tests: 43ms 1000000 tests: 46ms

If we assume that almost all non-SHA scripts aren't exactly 40 characters, we can add a length test:

internal static bool IsSHA1(string? script) => script is not null && script.Length == 40 && sha1.IsMatch(script);

and we get:

1000000 tests: 0ms 1000000 tests: 0ms 1000000 tests: 0ms 1000000 tests: 0ms 1000000 tests: 0ms 1000000 tests: 0ms 1000000 tests: 0ms 1000000 tests: 0ms 1000000 tests: 0ms 1000000 tests: 0ms

I'd much rather do that, and keep the API simple; thoughts?

[mgravell]

(FWIW: I experimented with a manually rolled test of the actual character test - no significant improvement, so: no point rewriting that IMO - just leave it as regex; it turns out that regex has been optimized pretty well for this type of scenario)

[martintmk]

Re key prefix; the existing implementation also didn't use any privileged APIs - it just expanded them as part of ExtractParameters - so IMO we should just keep doing that exactly the same

Re LoadedLuaScript at all; we can't remove it because of API signature breaking; it does still at least usually offer some benefit re pre-load of the script; not a massive benefit, perhaps, but: ... I'm not sure it is worth doing much in either direction here, i.e. I'm not convinced we should mark it [Obsolete] either.

How about we do these changes:

• Given that there is no real perf benefit based on your measurements the LoadedLuaScript.Evaluate just passes the string to evaluate the method. The hash is just ignored. This makes it resilient to Redis restarts.
• Can we hide it from the API? At least it will not attract some unnecessary attention.

[mgravell]

If you're asking about hiding the hash property: yes, we can safely IMO hide that in every way - [Browsable], [EditorBrowsable] and [Obsolete] (with the latter as a warning only, not an error). Sound good?

[martintmk]

If you're asking about hiding the hash property: yes, we can safely IMO hide that in every way - [Browsable], [EditorBrowsable] and [Obsolete] (with the latter as a warning only, not an error). Sound good?

And what do you think about hiding everything around LoadedLuaScript? Or is that too much?

The only real added value is that script pre-load which can be done by:

server.ScriptLoad(LuaScript.ExecutableScript, flags)

[mgravell]

It's a great question, but I think I'd rather defer on that for now

[martintmk]

It's a great question, but I think I'd rather defer on that for now

Sounds good, pushed the changes and reverted any API additions. Let me know if the approach is OK, then I'll focus on tests.

[mgravell]

Can you please add the .Length == 40 check as per the comments above? thanks

[martintmk]

Can you please add the .Length == 40 check as per the comments above? thanks

Done.

[martintmk]

@mgravell Pushed the changes. Anything left to address?

Unfortunately, some random tests failed on Ubuntu should I just retry?