Add check min-slave-* feature when evaluating Lua scripts and Functions by noradomi · Pull Request #10160 · redis/redis

noradomi · 2022-01-21T14:30:48Z

What problem does this PR solve?

Reference this issue:: #9993
Currently, there is no check min-slave-* config when evaluating Lua script.

Before the fix:

127.0.0.1:7000> eval "return redis.call('set','P','value')" 0
OK

After the fix:

127.0.0.1:7000> eval "return redis.call('set','P','value')" 0
(error) ERR Error running script (call to d05e85801446f5c3b3262aa9928ebf556baf9d64): @user_script:1: @user_script: 1: -NOREPLICAS Not enough good replicas to write.

Check List

Tests

Tests added in integration-4.tcl.

Code changes

Add check enough good slaves for write command when evaluating scripts.
This check is made before the script is executed, if we have function flags, and per redis command if we don't.

To Do

in functions or eval with shebang, unless no-writes flags is set prevent the script from running in the first place

…ript.

tests/integration/replication-4.tcl

anhldbk · 2022-01-24T15:04:10Z

@oranagra PTAL

oranagra

The fix LGTM (@MeirShpilraien PTAL).

maybe there's a way to combine these tests with existing tests that check this mechanism?
i.e. there are already tests that set up this state, and check normal redis commands, so all we need is to add an additional check for a script.

MeirShpilraien · 2022-01-24T22:34:10Z

@oranagra change looks good but I wonder if maybe we should not even run the script in such case unless no-writes flags is set (then we need this fix only for Lua script that do not use the flags for backward compatibility). WDYT?

anhldbk · 2022-01-25T02:27:26Z

@oranagra can this PR be merged into the latest 6.x branch?

oranagra · 2022-01-25T06:22:48Z

@MeirShpilraien i agree, we should have both checks, one when we know the write flag in advance, and one when we don't.
in theory, the second check could be bad since it can break the atomicity of the script, but in fact the replicas state can't change during the execution of the script, so either it'l reject the very first write, or none.

@anhldbk @noradomi we're currently in the process of releasing 7.0 RC1 and are busy with other things (which is why i didn't review it for a while).
regarding 6.x, since 7.0 (unstable) has a lot of changes around this area, backporting a commit isn't trivial.
i suggest that after this one is merged, we'll make a separate PR against the 6.2 branch.

noradomi · 2022-01-25T07:02:11Z

@oranagra maybe there's a way to combine these tests with existing tests that check this mechanism?

yeap, I refactored tests and combined these tests with existing tests for both normal commands and scripting.

tests/integration/replication-4.tcl

oranagra · 2022-01-25T14:00:10Z

@noradomi i added a TODO at the top, let me know if you're up to it, if not, we can open a separate issue.

noradomi · 2022-01-26T14:07:02Z

@oranagra Sorry for the late reply, about the TODO, I haven't done it yet. So I think you can open another issue for it and I will try to contribute when I clearly understand the context about it. (ex: functions, no-scripts flag ?)

anhldbk · 2022-01-28T03:19:15Z

@noradomi LGTM.

anhldbk · 2022-01-29T15:12:03Z

@oranagra LGTY?

oranagra

fix and tests LGTM.
as i noted (in the todo i added to the top), there's more to do in order to make this complete.
i'll look into it after 7.0 RC1 is released.

…ve-to-write-check

…ting the script

oranagra · 2022-02-01T13:38:01Z

@MeirShpilraien please review my last commit.

anyone wanna make a PR for redis 6.2 branch?

src/script.c

MeirShpilraien

Looks good (small comment, not a blocker).

Co-authored-by: Meir Shpilraien (Spielrein) <[email protected]>

anhldbk · 2022-02-07T02:34:41Z

@noradomi pls make another PR for branch 6.2

anhldbk · 2022-02-07T02:35:22Z

Thank you @oranagra @MeirShpilraien. You make my day.

noradomi · 2022-02-07T02:48:04Z

@noradomi pls make another PR for branch 6.2

yes, I will. Thanks @oranagra @MeirShpilraien for your time.

Phuc. Vo Trong added 2 commits January 21, 2022 21:15

Add check enough good slaves for write command when evaluating Lua sc…

71aaafa

…ript.

Test for min-slave-* feature when evaluate Lua script.

d84e6fd

noradomi mentioned this pull request Jan 21, 2022

[BUG] Config min-slaves-to-write is not worked with Lua scripts #9993

Closed

anhldbk reviewed Jan 21, 2022

View reviewed changes

tests/integration/replication-4.tcl Outdated Show resolved Hide resolved

oranagra added the 7.0-must-have label Jan 21, 2022

oranagra linked an issue Jan 21, 2022 that may be closed by this pull request

[BUG] Config min-slaves-to-write is not worked with Lua scripts #9993

Closed

Test for READ command case

fb3fb75

oranagra reviewed Jan 24, 2022

View reviewed changes

Combine tests for normal command and scripting command.

ead7fd2

oranagra reviewed Jan 25, 2022

View reviewed changes

tests/integration/replication-4.tcl Outdated Show resolved Hide resolved

tests/integration/replication-4.tcl Outdated Show resolved Hide resolved

Refactor tests in replication-4.tcl

e7274b0

oranagra reviewed Jan 25, 2022

View reviewed changes

tests/integration/replication-4.tcl Outdated Show resolved Hide resolved

Fix tests order

7b3d802

Remove unnecessary check in test

44435ed

oranagra approved these changes Jan 29, 2022

View reviewed changes

oranagra added 2 commits February 1, 2022 14:51

Merge remote-tracking branch 'origin/unstable' into scripting-min-sla…

fe01a54

…ve-to-write-check

add check for function or eval that's declared as write, before execu…

8550933

…ting the script

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Feb 1, 2022

MeirShpilraien reviewed Feb 3, 2022

View reviewed changes

src/script.c Outdated Show resolved Hide resolved

MeirShpilraien approved these changes Feb 3, 2022

View reviewed changes

Update comment

dc70c02

Co-authored-by: Meir Shpilraien (Spielrein) <[email protected]>

oranagra approved these changes Feb 3, 2022

View reviewed changes

oranagra changed the title ~~Add check min-slave-* feature when evaluating Lua script~~ Add check min-slave-* feature when evaluating Lua scripts and Functions Feb 3, 2022

oranagra merged commit 53c43fc into redis:unstable Feb 3, 2022

noradomi mentioned this pull request Feb 7, 2022

Add check min-slave-* feature when evaluating Lua scripts for 6.2 #10249

Merged

oranagra mentioned this pull request Feb 28, 2022

Release 7.0 RC2 #10355

Merged

This was referenced Apr 27, 2022

Redis 6.2.7 #10653

Closed

Redis 6.2.7 #10654

Merged

oranagra mentioned this pull request Jan 16, 2023

Redis 6.0.17 #11716

Merged

Conversation

noradomi commented Jan 21, 2022 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

Check List

Uh oh!

Uh oh!

anhldbk commented Jan 24, 2022

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

MeirShpilraien commented Jan 24, 2022

Uh oh!

anhldbk commented Jan 25, 2022

Uh oh!

oranagra commented Jan 25, 2022

Uh oh!

noradomi commented Jan 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Jan 25, 2022

Uh oh!

noradomi commented Jan 26, 2022

Uh oh!

anhldbk commented Jan 28, 2022

Uh oh!

anhldbk commented Jan 29, 2022

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra commented Feb 1, 2022

Uh oh!

Uh oh!

MeirShpilraien left a comment

Choose a reason for hiding this comment

Uh oh!

anhldbk commented Feb 7, 2022

Uh oh!

anhldbk commented Feb 7, 2022

Uh oh!

noradomi commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

noradomi commented Jan 21, 2022 •

edited by oranagra

Loading

noradomi commented Feb 7, 2022 •

edited

Loading