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

martintmk · 2022-06-23T13:48:14Z

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 · 2022-06-29T15:41:50Z

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 · 2022-07-01T15:57:53Z

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 · 2022-07-04T09:35:26Z

@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):

StackExchange.Redis/src/StackExchange.Redis/RedisDatabase.cs

Line 4804 in d6e05f6

physical.WriteHeader(RedisCommand.EVALSHA, 2 + keys.Length + values.Length);

The only remaining problem is the withKeyPrefix parameter, which is not currently accepted by the IDatabase API:

StackExchange.Redis/src/StackExchange.Redis/LuaScript.cs

Line 265 in d6e05f6

    
           public RedisResult Evaluate(IDatabase db, object? ps = null, RedisKey? withKeyPrefix = null, CommandFlags flags = CommandFlags.None)

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 · 2022-07-04T09:57:05Z

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 · 2022-07-04T10:09:09Z

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 · 2022-07-04T10:15:49Z

src/StackExchange.Redis/RedisDatabase.cs

+
+            try
+            {
+                return await ExecuteAsync(msg, ResultProcessor.ScriptResult, defaultValue: RedisResult.NullSingle);


I just copied the ScriptEvaluate, however not sure if the fallback is even needed here.

When debugging and restarting the Redis the catch branch was never reached and we recovered normally.

martintmk · 2022-07-12T12:02:13Z

@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 · 2022-08-16T15:30:10Z

src/StackExchange.Redis/Interfaces/IDatabase.cs

        /// <param name="flags">The flags to use for this operation.</param>
        /// <returns>A dynamic representation of the script's result.</returns>
-        /// <remarks><seealso href="https://redis.io/commands/evalsha"/></remarks>
+        /// <remarks>Please do not use this overload as it's not resilient to Redis server restarts. Use <see cref="ScriptEvaluate(string, RedisKey[], RedisValue[], CommandFlags)"/> instead.</remarks>


I think it is fine to offer this as an advisory comment, but: I'd hesitate to say "please do not" - more just something that gives relevant warning; ditto on the EditorBrowsableState.Never

src/StackExchange.Redis/Interfaces/IDatabase.cs

mgravell · 2022-08-16T15:50:10Z

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 · 2022-08-16T15:59:45Z

(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 · 2022-08-17T12:07:27Z

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 · 2022-08-17T12:56:32Z

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 · 2022-08-17T13:26:45Z

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 · 2022-08-17T14:27:32Z

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

martintmk · 2022-08-17T14:48:18Z

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

approach looks good 👍

mgravell · 2022-08-18T13:57:31Z

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

martintmk · 2022-08-22T12:10:57Z

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

Done.

martintmk · 2022-08-22T12:19:04Z

@mgravell Pushed the changes. Anything left to address?

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

NickCraver

Current looks good - thanks for the iterations on this to make it non-breaking and beneficial at the same time :)

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

511ec12

martintmk added 2 commits July 4, 2022 11:50

PR comments

47a835b

Update the docs

489d8b3

PR comments

a735312

add ConfigureAwait

1971d56

martintmk commented Jul 4, 2022

View reviewed changes

martintmk added 3 commits July 4, 2022 13:45

Test fixes

d1ff605

Cleanup

32cfafc

simplify the test

955d945

Alllow using hash and also support recovery for LoadedLuaScript

93a3f57

mgravell reviewed Aug 16, 2022

View reviewed changes

src/StackExchange.Redis/Interfaces/IDatabase.cs Outdated Show resolved Hide resolved

PR comments

fc19e50

martintmk added 2 commits August 17, 2022 16:40

PR comments

04f82b3

Cleanup

c3f7ab4

mgravell reviewed Aug 18, 2022

View reviewed changes

martintmk added 2 commits August 22, 2022 13:50

Speed-up the sha1 detection

912dcf9

Add tests

db0eef4

martintmk closed this Aug 22, 2022

martintmk reopened this Aug 22, 2022

NickCraver added 4 commits August 24, 2022 09:28

Merge remote-tracking branch 'origin/main' into pr/2170

96fb7da

Cleanup comments and usages

036fb57

Add release notes

6876197

Fix test formatting

6962655

NickCraver approved these changes Aug 24, 2022

View reviewed changes

NickCraver merged commit fdc90e9 into StackExchange:main Aug 24, 2022

NickCraver mentioned this pull request Feb 12, 2023

Automatically detect and reupload flushed LUA script #1968

Closed

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

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

Uh oh!

Conversation

martintmk commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgravell commented Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martintmk commented Jul 1, 2022

Uh oh!

martintmk commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgravell commented Jul 4, 2022

Uh oh!

martintmk commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martintmk Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martintmk commented Jul 12, 2022

Uh oh!

mgravell Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

martintmk Aug 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mgravell commented Aug 16, 2022

Uh oh!

mgravell commented Aug 16, 2022

Uh oh!

martintmk commented Aug 17, 2022

Uh oh!

mgravell commented Aug 17, 2022

Uh oh!

martintmk commented Aug 17, 2022

Uh oh!

mgravell commented Aug 17, 2022

Uh oh!

martintmk commented Aug 17, 2022

Uh oh!

mgravell left a comment

Choose a reason for hiding this comment

Uh oh!

mgravell commented Aug 18, 2022

Uh oh!

martintmk commented Aug 22, 2022

Uh oh!

martintmk commented Aug 22, 2022

Uh oh!

NickCraver left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

martintmk commented Jun 23, 2022 •

edited

Loading

mgravell commented Jun 29, 2022 •

edited

Loading

martintmk commented Jul 4, 2022 •

edited

Loading

martintmk commented Jul 4, 2022 •

edited

Loading

martintmk Jul 4, 2022 •

edited

Loading