You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/08/20 15:33:12 UTC
[GitHub] [incubator-kvrocks] xiaobiaozhao opened a new pull request, #782: add eval
xiaobiaozhao opened a new pull request, #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782
This is design https://github.com/apache/incubator-kvrocks/discussions/720
useage
`127.0.0.1:6666> EVAL_RO "redis.call('SET', 'a', 'bb') return {ARGV[1], redis.read_only}" 0 hello
(error) ERR running script (call to f_963e3d4d3ef14756826239b888ba1c2257052a58): @user_script:1: Write commands are not allowed from read-only scripts`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1252215515
@PragmaTwice this manner seems by design for me O_O
Generally, Kvrocks doesn't employ a DBMS ACID model so "phantom read" is not well defined in this scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1252235159
> > @PragmaTwice this manner seems by design for me O_O
> > Generally, Kvrocks doesn't employ a DBMS ACID model so "phantom read" is not well defined in this scope.
>
> Got it. maybe I need to read the transaction chapter of DDIA again : (
My bad. 😆 We have no ACID, so just the behavior likes phantom read.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] datavisorxiaobiaozhao commented on a diff in pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
datavisorxiaobiaozhao commented on code in PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#discussion_r952578855
##########
src/redis_cmd.cc:
##########
@@ -4742,6 +4742,32 @@ class CommandEvalSHA : public Commander {
}
};
+class CommandEvalRO : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ return Status::OK();
+ }
+
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1250188252
> @xiaobiaozhao Can help to resolve conflicts and take a look at why the CI NOT works.
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#discussion_r959064734
##########
src/scripting.cc:
##########
@@ -898,7 +921,9 @@ Status createFunction(Server *srv, const std::string &body, std::string *sha) {
funcdef += body;
funcdef += "\nend";
- lua_State *lua = srv->Lua();
+ if (!lua) {
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1252187478
@ShooterIT @PragmaTwice @PragmaTwice This PR will be merged in tomorrow if we have no more comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#discussion_r956556420
##########
tests/tcl/tests/unit/scripting.tcl:
##########
@@ -387,6 +387,17 @@ start_server {tags {"scripting"}} {
set v [r eval { return redis.log(redis.LOG_WARNING, 'warning level'); } 0]
assert_equal "" $v
} {}
+
+ test {EVAL_RO - Successful case} {
+ r set foo bar
+ assert_equal bar [r eval_ro {return redis.call('get', KEYS[1]);} 1 foo]
+ }
+
+ test {EVAL_RO - Cannot run write commands} {
+ r set foo bar
+ catch {r eval_ro {redis.call('del', KEYS[1]);} 1 foo} e
+ set e
+ } {ERR Write commands are not allowed from read-only scripts*}
Review Comment:
added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#discussion_r956556446
##########
src/scripting.cc:
##########
@@ -233,11 +240,18 @@ namespace Lua {
Status evalGenericCommand(Redis::Connection *conn,
const std::vector<std::string> &args,
bool evalsha,
- std::string *output) {
+ std::string *output,
+ bool read_only) {
int64_t numkeys = 0;
char funcname[43];
Server *srv = conn->GetServer();
- lua_State *lua = srv->Lua();
+ lua_State *lua = NULL;
+ if (read_only) {
+ // use worker's lua VM
+ lua = conn->Owner()->Lua();
+ } else {
+ lua = srv->Lua();
+ }
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1252212518
> Another issue is phantom read in read-only mode but it can't be solved it in this PR:
>
> For example:
>
> ```
> Time Worker A Worker B
>
> T1 Get K0
>
> T2 Set K0 V0'
>
> T3 Get K0
> ```
Has this issue been resolved now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1252213346
@git-hulk this patch has a code conflict with #900. But I can simply update after this patch merged :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1240138981
@xiaobiaozhao would you like to rebase this patch onto latest unstable? Then I can find some time to review it :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#discussion_r976003091
##########
tests/gocase/unit/command/command_test.go:
##########
@@ -35,13 +35,6 @@ func TestCommand(t *testing.T) {
rdb := srv.NewClient()
defer func() { require.NoError(t, rdb.Close()) }()
- t.Run("Kvrocks supports 183 commands currently", func(t *testing.T) {
- r := rdb.Do(ctx, "COMMAND", "COUNT")
- v, err := r.Int()
- require.NoError(t, err)
- require.Equal(t, 183, v)
- })
-
Review Comment:
@git-hulk do we remove this test intentionally?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#discussion_r976008197
##########
tests/gocase/unit/command/command_test.go:
##########
@@ -35,13 +35,6 @@ func TestCommand(t *testing.T) {
rdb := srv.NewClient()
defer func() { require.NoError(t, rdb.Close()) }()
- t.Run("Kvrocks supports 183 commands currently", func(t *testing.T) {
- r := rdb.Do(ctx, "COMMAND", "COUNT")
- v, err := r.Int()
- require.NoError(t, err)
- require.Equal(t, 183, v)
- })
-
Review Comment:
Get it. Although, the code conflict doesn't relate to this test but (1) I'm porting scripting tests (2) this PR added new scripting tests XD.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1252227937
> @git-hulk this patch has a code conflict with #900. But I can simply update after this patch merged :)
Okay, thanks. I think we can remove the command count test case since it's meaningless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1250192207
Thanks, will take a look soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1253131363
@git-hulk @PragmaTwice the unstable go cases can be resolved by https://github.com/apache/incubator-kvrocks/pull/899. Please help with reviewing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun merged pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1223441613
Another issue is phantom read in read-only mode but it can't be solved it in this PR:
For example:
```
Time Worker A Worker B
T1 Get K0
T2 Set K0 V0'
T3 Get K0
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#discussion_r951655980
##########
tests/tcl/tests/unit/scripting.tcl:
##########
@@ -387,6 +387,17 @@ start_server {tags {"scripting"}} {
set v [r eval { return redis.log(redis.LOG_WARNING, 'warning level'); } 0]
assert_equal "" $v
} {}
+
+ test {EVAL_RO - Successful case} {
+ r set foo bar
+ assert_equal bar [r eval_ro {return redis.call('get', KEYS[1]);} 1 foo]
+ }
+
+ test {EVAL_RO - Cannot run write commands} {
+ r set foo bar
+ catch {r eval_ro {redis.call('del', KEYS[1]);} 1 foo} e
+ set e
+ } {ERR Write commands are not allowed from read-only scripts*}
Review Comment:
Consider to add test for `evalsha_ro`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#discussion_r952066157
##########
src/redis_cmd.cc:
##########
@@ -4742,6 +4742,32 @@ class CommandEvalSHA : public Commander {
}
};
+class CommandEvalRO : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ return Status::OK();
+ }
+
Review Comment:
```suggestion
```
Can remove this part if it does nothing
##########
src/scripting.cc:
##########
@@ -898,7 +921,9 @@ Status createFunction(Server *srv, const std::string &body, std::string *sha) {
funcdef += body;
funcdef += "\nend";
- lua_State *lua = srv->Lua();
+ if (!lua) {
Review Comment:
Can we choose the Lua state in caller? the caller should determine where to create the function.
##########
src/scripting.cc:
##########
@@ -233,11 +240,18 @@ namespace Lua {
Status evalGenericCommand(Redis::Connection *conn,
const std::vector<std::string> &args,
bool evalsha,
- std::string *output) {
+ std::string *output,
+ bool read_only) {
int64_t numkeys = 0;
char funcname[43];
Server *srv = conn->GetServer();
- lua_State *lua = srv->Lua();
+ lua_State *lua = NULL;
+ if (read_only) {
+ // use worker's lua VM
+ lua = conn->Owner()->Lua();
+ } else {
+ lua = srv->Lua();
+ }
Review Comment:
```suggestion
lua_State *lua = srv->Lua();
if (read_only) {
// Use the worker's private Lua VM when entering the read-only mode
lua = conn->Owner()->Lua();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1248074256
@xiaobiaozhao Can help to resolve conflicts and take a look at why the CI NOT works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1250188199
>
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1252224272
> > Another issue is phantom read in read-only mode but it can't be solved it in this PR:
> > For example:
> > ```
> > Time Worker A Worker B
> >
> > T1 Get K0
> >
> > T2 Set K0 V0'
> >
> > T3 Get K0
> > ```
>
> Has this issue been resolved now?
This issue is NOT introduced by this PR and can't be resolved in short time. We need to support the version read mechanism in all readonly commands.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1252231213
> @PragmaTwice this manner seems by design for me O_O
>
> Generally, Kvrocks doesn't employ a DBMS ACID model so "phantom read" is not well defined in this scope.
Got it. maybe I need to read the transaction chapter of DDIA again : (
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #782: Add EVAL_RO
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #782:
URL: https://github.com/apache/incubator-kvrocks/pull/782#discussion_r976004115
##########
tests/gocase/unit/command/command_test.go:
##########
@@ -35,13 +35,6 @@ func TestCommand(t *testing.T) {
rdb := srv.NewClient()
defer func() { require.NoError(t, rdb.Close()) }()
- t.Run("Kvrocks supports 183 commands currently", func(t *testing.T) {
- r := rdb.Do(ctx, "COMMAND", "COUNT")
- v, err := r.Int()
- require.NoError(t, err)
- require.Equal(t, 183, v)
- })
-
Review Comment:
Yes, as said in comment: https://github.com/apache/incubator-kvrocks/pull/782#issuecomment-1252227937, this test case is not much help for us and it causes conflicts frequently, so I think we should remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org